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

"Expression value is unused" should not be reported for last expression in notebook cell #4164

Merged
merged 8 commits into from
Nov 11, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/pyright-internal/src/analyzer/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1534,6 +1534,18 @@ export class Checker extends ParseTreeWalker {
}
}

if (
reportAsUnused &&
this._fileInfo.ipythonMode === IPythonMode.CellDocs &&
node.parent?.nodeType === ParseNodeType.StatementList &&
node.parent.parent?.nodeType === ParseNodeType.Module &&
node.parent.statements[node.parent.statements.length - 1] === node
debonte marked this conversation as resolved.
Show resolved Hide resolved
) {
// Exclude an expression at the end of a notebook cell, as that is treated as
// the cell's value.
reportAsUnused = false;
debonte marked this conversation as resolved.
Show resolved Hide resolved
}

if (reportAsUnused) {
this._evaluator.addDiagnostic(
this._fileInfo.diagnosticRuleSet.reportUnusedExpression,
Expand Down
2 changes: 1 addition & 1 deletion packages/pyright-internal/src/analyzer/sourceFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1357,7 +1357,7 @@ export class SourceFile {
}

test_enableIPythonMode(enable: boolean) {
this._ipythonMode = enable ? IPythonMode.ConcatDoc : IPythonMode.None;
this._ipythonMode = enable ? IPythonMode.CellDocs : IPythonMode.None;
}

private _buildFileInfo(
Expand Down
24 changes: 12 additions & 12 deletions packages/pyright-internal/src/tests/harness/fourslash/testState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,7 @@ export class TestState {
}

verifyDiagnostics(map?: { [marker: string]: { category: string; message: string } }): void {
this._analyze();
this.analyze();

// organize things per file
const resultPerFile = this._getDiagnosticsPerFile();
Expand Down Expand Up @@ -671,7 +671,7 @@ export class TestState {
): Promise<any> {
// make sure we don't use cache built from other tests
this.workspace.serviceInstance.invalidateAndForceReanalysis();
this._analyze();
this.analyze();

for (const range of this.getRanges()) {
const name = this.getMarkerName(range.marker!);
Expand Down Expand Up @@ -737,7 +737,7 @@ export class TestState {
}

async verifyCommand(command: Command, files: { [filePath: string]: string }): Promise<any> {
this._analyze();
this.analyze();

const commandResult = await this._hostSpecificFeatures.execute(
new TestLanguageService(this.workspace, this.console, this.fs),
Expand Down Expand Up @@ -923,7 +923,7 @@ export class TestState {
},
verifyCodeActionCount?: boolean
): Promise<any> {
this._analyze();
this.analyze();

for (const range of this.getRanges()) {
const name = this.getMarkerName(range.marker!);
Expand Down Expand Up @@ -1076,7 +1076,7 @@ export class TestState {
},
abbrMap?: { [abbr: string]: AbbreviationInfo }
): Promise<void> {
this._analyze();
this.analyze();

for (const marker of this.getMarkers()) {
const markerName = this.getMarkerName(marker);
Expand Down Expand Up @@ -1246,7 +1246,7 @@ export class TestState {
};
}
): void {
this._analyze();
this.analyze();

for (const marker of this.getMarkers()) {
const fileName = marker.fileName;
Expand Down Expand Up @@ -1315,7 +1315,7 @@ export class TestState {
references: DocumentRange[];
};
}) {
this._analyze();
this.analyze();

for (const marker of this.getMarkers()) {
const fileName = marker.fileName;
Expand Down Expand Up @@ -1365,7 +1365,7 @@ export class TestState {
references: DocumentHighlight[];
};
}) {
this._analyze();
this.analyze();

for (const name of Object.keys(map)) {
const marker = this.getMarkerByName(name);
Expand Down Expand Up @@ -1397,7 +1397,7 @@ export class TestState {
},
filter: DefinitionFilter = DefinitionFilter.All
) {
this._analyze();
this.analyze();

for (const marker of this.getMarkers()) {
const fileName = marker.fileName;
Expand Down Expand Up @@ -1425,7 +1425,7 @@ export class TestState {
definitions: DocumentRange[];
};
}) {
this._analyze();
this.analyze();

for (const marker of this.getMarkers()) {
const fileName = marker.fileName;
Expand Down Expand Up @@ -1454,7 +1454,7 @@ export class TestState {
changes: FileEditAction[];
};
}) {
this._analyze();
this.analyze();

for (const marker of this.getMarkers()) {
const fileName = marker.fileName;
Expand Down Expand Up @@ -1798,7 +1798,7 @@ export class TestState {
return position <= editStart ? position : position < editEnd ? -1 : position + length - +(editEnd - editStart);
}

private _analyze() {
public analyze() {
while (this.program.analyze()) {
// Continue to call analyze until it completes. Since we're not
// specifying a timeout, it should complete the first time.
Expand Down
33 changes: 33 additions & 0 deletions packages/pyright-internal/src/tests/ipythonMode.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -560,3 +560,36 @@ function findCommentByOffset(tokens: TextRangeCollection<Token>, offset: number)

return comment;
}

test('unused expression at end is not error', async () => {
const code = `
// @filename: test.py
// @ipythonMode: true
//// 4[|/*marker*/|]
`;

verifyAnalysisDiagnosticCount(code, 0);
});

test('unused expression is error if within another statement', async () => {
const code = `
// @filename: test.py
// @ipythonMode: true
//// if True:
//// 4[|/*marker*/|]
`;

verifyAnalysisDiagnosticCount(code, 1);
});

function verifyAnalysisDiagnosticCount(code: string, expectedCount: number) {
debonte marked this conversation as resolved.
Show resolved Hide resolved
const state = parseAndGetTestState(code).state;

state.analyze();

const range = state.getRangeByMarkerName('marker')!;
const source = state.program.getBoundSourceFile(range.fileName)!;
const diagnostics = source.getDiagnostics(state.configOptions);

assert.strictEqual(diagnostics?.length, expectedCount);
}