sourcegraph / scip-python

SCIP indexer for Python
Other
46 stars 21 forks source link

Assignments have `ReadAccess` symbol role #141

Open crackcomm opened 7 months ago

crackcomm commented 7 months ago

In this code:

class MyClass:
    var = "value"

def class_abuser():
    MyClass.var = "test"

Both occurrences have an incorrect symbol role of read access.

I hacked a patch for my use case:

diff --git a/packages/pyright-scip/src/treeVisitor.ts b/packages/pyright-scip/src/treeVisitor.ts
index ada07dd8d..e292f2aa3 100644
--- a/packages/pyright-scip/src/treeVisitor.ts
+++ b/packages/pyright-scip/src/treeVisitor.ts
@@ -1,6 +1,6 @@
 import * as path from 'path';
 import { AnalyzerFileInfo } from 'pyright-internal/analyzer/analyzerFileInfo';
-import { getFileInfo, getImportInfo } from 'pyright-internal/analyzer/analyzerNodeInfo';
+import { getFileInfo, getFlowNode, getImportInfo } from 'pyright-internal/analyzer/analyzerNodeInfo';
 import { ParseTreeWalker } from 'pyright-internal/analyzer/parseTreeWalker';
 import { TypeEvaluator } from 'pyright-internal/analyzer/typeEvaluatorTypes';
 import { convertOffsetToPosition } from 'pyright-internal/common/positionUtils';
@@ -41,6 +41,7 @@ import {
     DeclarationType,
     isAliasDeclaration,
     isIntrinsicDeclaration,
+    isVariableDeclaration,
 } from 'pyright-internal/analyzer/declaration';
 import { ConfigOptions, ExecutionEnvironment } from 'pyright-internal/common/configOptions';
 import { versionToString } from 'pyright-internal/common/pythonVersion';
@@ -54,6 +55,7 @@ import { HoverResults } from 'pyright-internal/languageService/hoverProvider';
 import { convertDocStringToMarkdown } from 'pyright-internal/analyzer/docStringConversion';
 import { assert } from 'pyright-internal/common/debug';
 import { getClassFieldsRecursive } from 'pyright-internal/analyzer/typeUtils';
+import { FlowFlags } from 'pyright-internal/analyzer/codeFlowTypes';

 //  Useful functions for later, but haven't gotten far enough yet to use them.
 //      extractParameterDocumentation
@@ -573,6 +575,12 @@ export class TreeVisitor extends ParseTreeWalker {
             return this.emitDeclarationWithoutNode(node, decl);
         }

+        if (isVariableDeclaration(decl) && getFlowNode(decl.node)!?.flags & FlowFlags.Assignment) {
+            const symbol = this.getScipSymbol(decl.node);
+            this.pushNewOccurrence(node, symbol, scip.SymbolRole.WriteAccess);
+            return true;
+        }
+
         const existingSymbol = this.rawGetLsifSymbol(decl.node);
         if (existingSymbol) {
             if (decl.node.id === parent.id || decl.node.id === node.id) {
varungandhi-src commented 7 months ago

You're correct in that it should not be omitting WriteAccess here. However, using only WriteAccess is not strictly correct as a read may also be performed depending on how the property was declared. For example, when using the @property decorator, if you use @myprop.setter, then it receives self as the first argument, which allows it to perform a read of the old value if it wants to. So this requires a more complex check.