Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add reportUnreachable option #34

Merged
merged 12 commits into from
Jan 24, 2024
4 changes: 3 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"editor.formatOnSave": true
},
"editor.defaultFormatter": "esbenp.prettier-vscode",
"typescript.enablePromptUseWorkspaceTsdk": true,
"typescript.tsdk": "node_modules/typescript/lib",
"python.languageServer": "None",
"editor.formatOnSave": true,
Expand Down Expand Up @@ -58,5 +59,6 @@
"search.exclude": {
"pw": true
},
"python.terminal.activateEnvInCurrentTerminal": true
"python.terminal.activateEnvInCurrentTerminal": true,
"jest.autoRun": {},
}
15 changes: 15 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,21 @@ pyright currently has no way to pin the version used by vscode, which means if t

python developers should not be expected to have to install nodejs in order to typecheck their python code. it should just be a regular pypi package, just mypy, ruff, and pretty much every other python command line tool

### issues with unreachable code

pyright often incorrectly marks code as unreachable. in most cases, unreachable code is a mistake and therefore should be an error, but pyright does not have an option to report unreachable code. in fact, unreachable code is not even type-checked at all:

```py
if sys.platform == "win32":
1 + "" # no error
```

by default, pyright will treat the body in the code above as unreachable if pyright itself was run on an operating system other than windows. this is bad of course, because chances are if you write such a check, you intend for your code to be executed on multiple platforms.

to make things worse, unreachable code is not even type-checked, so the obviously invalid `1 + ""` above will go completely unnoticed by the type checker.

basedpyright solves this issue with a `reportUnreachable` option, which will report an error on any code that it thinks cannot be reached. from there, you can either [update your pyright config to specify more platforms using the `pythonPlatform` option](https://github.com/detachhead/basedpyright/blob/main/docs/configuration.md#main-configuration-options) if you intend for the code to be reachable.

### basedmypy feature parity

[basedmypy](https://github.com/kotlinisland/basedmypy) is a fork of mypy with a similar goal in mind: to fix some of the serious problems in mypy that do not seem to be a priority for the maintainers. it also adds many new features which may not be standardized but greatly improve the developer experience when working with python's far-from-perfect type system.
Expand Down
7 changes: 7 additions & 0 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,12 @@ The following settings control pyright’s diagnostic output (warnings or errors

<a name="reportShadowedImports"></a> **reportShadowedImports** [boolean or string, optional]: Generate or suppress diagnostics for files that are overriding a module in the stdlib. The default value for this setting is `"none"`.

## Based options

the following additional options are not available in regular pyright:

<a name="reportUnreachable"></a> **reportUnreachable** [boolean or string, optional]: Generate or suppress diagnostics for unreachable code. The default value for this setting is `"none"`.

## Execution Environment Options
Pyright allows multiple “execution environments” to be defined for different portions of your source tree. For example, a subtree may be designed to run with different import search paths or a different version of the python interpreter than the rest of the source base.

Expand Down Expand Up @@ -373,6 +379,7 @@ The following table lists the default severity levels for each diagnostic rule w
| reportUninitializedInstanceVariable | "none" | "none" | "none" | "none" |
| reportUnnecessaryTypeIgnoreComment | "none" | "none" | "none" | "none" |
| reportUnusedCallResult | "none" | "none" | "none" | "none" |
| reportUnreachable | "none" | "none" | "none" | "none" |


## Locale Configuration
Expand Down
30 changes: 26 additions & 4 deletions packages/pyright-internal/src/analyzer/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1325,6 +1325,14 @@ export class Binder extends ParseTreeWalker {
const elseLabel = this._createBranchLabel();
const postIfLabel = this._createBranchLabel(preIfFlowNode);

const notTypeCheckingNode: FlowNode = {
flags: FlowFlags.NotTypeChecking | FlowFlags.Unreachable,
id: getUniqueFlowNodeId(),
};

const isTypeCheckingNode = (node: ExpressionNode): node is NameNode =>
node.nodeType === ParseNodeType.Name && node.value === 'TYPE_CHECKING';

postIfLabel.affectedExpressions = this._trackCodeFlowExpressions(() => {
// Determine if the test condition is always true or always false. If so,
// we can treat either the then or the else clause as unconditional.
Expand All @@ -1339,16 +1347,30 @@ export class Binder extends ParseTreeWalker {
this._bindConditional(node.testExpression, thenLabel, elseLabel);

// Handle the if clause.
this._currentFlowNode =
constExprValue === false ? Binder._unreachableFlowNode : this._finishFlowLabel(thenLabel);
if (constExprValue === false) {
this._currentFlowNode =
node.testExpression.nodeType === ParseNodeType.UnaryOperation &&
node.testExpression.operator === OperatorType.Not &&
isTypeCheckingNode(node.testExpression.expression) &&
constExprValue === false
? notTypeCheckingNode
: Binder._unreachableFlowNode;
} else {
this._currentFlowNode = this._finishFlowLabel(thenLabel);
}
this.walk(node.ifSuite);
this._addAntecedent(postIfLabel, this._currentFlowNode);

// Now handle the else clause if it's present. If there
// are chained "else if" statements, they'll be handled
// recursively here.
this._currentFlowNode =
constExprValue === true ? Binder._unreachableFlowNode : this._finishFlowLabel(elseLabel);
if (constExprValue === true) {
this._currentFlowNode = isTypeCheckingNode(node.testExpression)
? notTypeCheckingNode
: Binder._unreachableFlowNode;
} else {
this._currentFlowNode = this._finishFlowLabel(elseLabel);
}
if (node.elseSuite) {
this.walk(node.elseSuite);
} else {
Expand Down
1 change: 1 addition & 0 deletions packages/pyright-internal/src/analyzer/codeFlowTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export enum FlowFlags {
FalseNeverCondition = 1 << 17, // Condition whose type evaluates to never when narrowed in negative test
NarrowForPattern = 1 << 18, // Narrow the type of the subject expression within a case statement
ExhaustedMatch = 1 << 19, // Control flow gate that is closed when match is provably exhaustive
NotTypeChecking = 1 << 20, // not TYPE_CHECKING block (ie. unreachable, but should not reportUnreachable)
}

let _nextFlowNodeId = 1;
Expand Down
1 change: 1 addition & 0 deletions packages/pyright-internal/src/analyzer/codeFlowUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ export function formatControlFlowGraph(flowNode: FlowNode) {
if (flags & FlowFlags.FalseNeverCondition) return 'FalseNever';
if (flags & FlowFlags.NarrowForPattern) return 'Pattern';
if (flags & FlowFlags.ExhaustedMatch) return 'Exhaust';
if (flags & FlowFlags.NotTypeChecking) return 'NotTypeChecking';
throw new Error();
}

Expand Down
21 changes: 19 additions & 2 deletions packages/pyright-internal/src/analyzer/typeEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2997,6 +2997,18 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
return codeFlowEngine.isFlowNodeReachable(flowNode, sourceFlowNode);
}

function isNotTypeCheckingBlock(node: ParseNode): boolean {
//TODO: abstract this logic, its used in isNodeReachable as well
const flowNode = AnalyzerNodeInfo.getFlowNode(node);
if (!flowNode) {
if (node.parent) {
return isNotTypeCheckingBlock(node.parent);
}
return false;
}
return !!(flowNode.flags & FlowFlags.NotTypeChecking);
}

function isAfterNodeReachable(node: ParseNode): boolean {
const returnFlowNode = AnalyzerNodeInfo.getAfterFlowNode(node);
if (!returnFlowNode) {
Expand Down Expand Up @@ -3089,7 +3101,12 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
function addUnreachableCode(node: ParseNode, textRange: TextRange) {
if (!isDiagnosticSuppressedForNode(node)) {
const fileInfo = AnalyzerNodeInfo.getFileInfo(node);
fileInfo.diagnosticSink.addUnreachableCodeWithTextRange(LocMessage.unreachableCode(), textRange);
const message = LocMessage.unreachableCode();
if (fileInfo.diagnosticRuleSet.reportUnreachable === 'none' || isNotTypeCheckingBlock(node)) {
fileInfo.diagnosticSink.addUnreachableCodeWithTextRange(message, textRange);
} else {
addDiagnostic(DiagnosticRule.reportUnreachable, message, node, textRange);
}
}
}

Expand All @@ -3106,7 +3123,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
node: ParseNode,
range?: TextRange
) {
if (!isDiagnosticSuppressedForNode(node) && isNodeReachable(node)) {
if (!isDiagnosticSuppressedForNode(node)) {
const fileInfo = AnalyzerNodeInfo.getFileInfo(node);
return fileInfo.diagnosticSink.addDiagnosticWithTextRange(diagLevel, message, range || node);
}
Expand Down
9 changes: 9 additions & 0 deletions packages/pyright-internal/src/common/configOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,11 @@ export interface DiagnosticRuleSet {

// Report missing @override decorator.
reportImplicitOverride: DiagnosticLevel;

// basedpyright options:

// Report unreachable code
reportUnreachable: DiagnosticLevel;
}

export function cloneDiagnosticRuleSet(diagSettings: DiagnosticRuleSet): DiagnosticRuleSet {
Expand Down Expand Up @@ -511,6 +516,7 @@ export function getOffDiagnosticRuleSet(): DiagnosticRuleSet {
reportMatchNotExhaustive: 'none',
reportShadowedImports: 'none',
reportImplicitOverride: 'none',
reportUnreachable: 'none',
};

return diagSettings;
Expand Down Expand Up @@ -596,6 +602,7 @@ export function getBasicDiagnosticRuleSet(): DiagnosticRuleSet {
reportMatchNotExhaustive: 'none',
reportShadowedImports: 'none',
reportImplicitOverride: 'none',
reportUnreachable: 'none',
};

return diagSettings;
Expand Down Expand Up @@ -681,6 +688,7 @@ export function getStandardDiagnosticRuleSet(): DiagnosticRuleSet {
reportMatchNotExhaustive: 'none',
reportShadowedImports: 'none',
reportImplicitOverride: 'none',
reportUnreachable: 'none',
};

return diagSettings;
Expand Down Expand Up @@ -766,6 +774,7 @@ export function getStrictDiagnosticRuleSet(): DiagnosticRuleSet {
reportMatchNotExhaustive: 'error',
reportShadowedImports: 'none',
reportImplicitOverride: 'none',
reportUnreachable: 'error',
};

return diagSettings;
Expand Down
3 changes: 3 additions & 0 deletions packages/pyright-internal/src/common/diagnosticRules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,7 @@ export enum DiagnosticRule {
reportMatchNotExhaustive = 'reportMatchNotExhaustive',
reportShadowedImports = 'reportShadowedImports',
reportImplicitOverride = 'reportImplicitOverride',

// basedpyright options:
reportUnreachable = 'reportUnreachable',
}
9 changes: 9 additions & 0 deletions packages/pyright-internal/src/tests/samples/unreachable2.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
from typing import TYPE_CHECKING

if not TYPE_CHECKING:
...

if TYPE_CHECKING:
...
else:
...
16 changes: 16 additions & 0 deletions packages/pyright-internal/src/tests/typeEvaluator1.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,22 @@ test('Unreachable1', () => {
TestUtils.validateResults(analysisResults, 0, 0, 2, 1, 4);
});

test('Unreachable1 reportUnreachable', () => {
const configOptions = new ConfigOptions(Uri.empty());
configOptions.diagnosticRuleSet.reportUnreachable = 'error';
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['unreachable1.py'], configOptions);

TestUtils.validateResults(analysisResults, 4, 0, 2, 1, 0);
});

test('Unreachable2 reportUnreachable TYPE_CHECKING', () => {
const configOptions = new ConfigOptions(Uri.empty());
configOptions.diagnosticRuleSet.reportUnreachable = 'error';
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['unreachable2.py'], configOptions);

TestUtils.validateResults(analysisResults, 0, 0, 0, 0, 2);
});

test('Builtins1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['builtins1.py']);

Expand Down
16 changes: 16 additions & 0 deletions packages/vscode-pyright/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1214,6 +1214,22 @@
true,
false
]
},
"reportUnreachable": {
"type": [
"string",
"boolean"
],
"description": "Diagnostics for unreachable code.",
"default": "none",
"enum": [
"none",
"information",
"warning",
"error",
true,
false
]
}
}
},
Expand Down
6 changes: 6 additions & 0 deletions packages/vscode-pyright/schemas/pyrightconfig.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,12 @@
"title": "Controls reporting overridden methods that are missing an `@override` decorator",
"default": "none"
},
"reportUnreachable": {
"$id": "#/properties/reportUnreachable",
"$ref": "#/definitions/diagnostic",
"title": "Controls reporting of unreachable code",
"default": "none"
},
"extraPaths": {
"$id": "#/properties/extraPaths",
"type": "array",
Expand Down
8 changes: 5 additions & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ skip-magic-trailing-comma = true
preview = true
force-exclude = "pw|pyright\\-internal"

[tool.pylint.MASTER]
[tool.pylint.MASTER]
fail-on = "I"
bad-names = ["foo", "bar", "baz", "retval"]
load-plugins = [
Expand Down Expand Up @@ -244,11 +244,12 @@ score = "no"
max-line-length = 200

[tool.pyright]
ignore = ["pw"]
ignore = ["pw", "basedpyright/dist", "packages"]
pythonVersion = "3.8"
pythonPlatform = "All"
include = ["basedpyright", "get_version.py", "pdm_build.py"]
exclude = ['basedpyright/dist']
exclude = ["pw", "basedpyright/dist", "packages"]
typeCheckingMode = "strict"
reportMissingTypeStubs = false
strictListInference = true
strictDictionaryInference = true
Expand Down Expand Up @@ -306,6 +307,7 @@ reportUnnecessaryTypeIgnoreComment = true
reportMatchNotExhaustive = true
reportImplicitOverride = true
reportShadowedImports = true
reportUnreachable = true

[tool.ruff]
unsafe-fixes = true
Expand Down
Loading