NRE in SymbolCollector.AddProperty

Microsoft.Python.Analysis.Analyzer.Symbols.SymbolCollector.AddProperty(FunctionDefinition, IPythonType, Boolean) in Analysis\Ast\Impl\Analyzer\Symbols\SymbolCollector.cs:line 170
Microsoft.Python.Analysis.Analyzer.Symbols.SymbolCollector.TryAddProperty(FunctionDefinition, IPythonType) in Analysis\Ast\Impl\Analyzer\Symbols\SymbolCollector.cs:line 158
Microsoft.Python.Analysis.Analyzer.Symbols.SymbolCollector.AddFunctionOrProperty(FunctionDefinition) in Analysis\Ast\Impl\Analyzer\Symbols\SymbolCollector.cs:line 101
Microsoft.Python.Analysis.Analyzer.Symbols.SymbolCollector.Walk(FunctionDefinition) in Analysis\Ast\Impl\Analyzer\Symbols\SymbolCollector.cs:line 77
Microsoft.Python.Parsing.Ast.FunctionDefinition.Walk(PythonWalker) in Parsing\Impl\Ast\FunctionDefinition.cs:line 197

Not sure what could be null at line 170, though. Maybe it's getting optimized and fd is null and is breaking at fd.Name.

null function names do happen, but walker checks it in Walk:

if (!string.IsNullOrEmpty(fd.Name)) {
    AddFunctionOrProperty(fd);

So I guess, _eval or something in _eval.GetLocationOfName(fd)

Was the same as #1219, I believe.

Didn't go away.

Alexander Sher (@AlexanderSher)

Walking AST, walker didn't crash so FunctionDefinition is valid. That's line 197.
Line 77 can only get called when if (!string.IsNullOrEmpty(fd.Name)) so both fd and Name are valid.
101 and 158 are just passing stuff down
177 is _eval.GetLocationOfName(fd).

So either a) _eval is null - which is unlikely, we'd crash way earlier and besides, it is readonly and

_eval = eval ?? throw new ArgumentNullException(nameof(eval));

this means GetLocationOfName is inlined and the issue is there. fd is not null and if it was, then GetLocationOfName would throw in

node = node ?? throw new ArgumentNullException(nameof(node));

so what IS null is the NameExpression in fd which may happen in

def (a, b, c)

OR if function is from a library module and somehow fd lost its name node. This is unlikely since we actually are running analysis and there is live AST right there. Now FunctionDefinition has

public override string/*!*/ Name => NameExpression.Name ?? string.Empty;

which then would crash on NameExpression being null which contradicts with the fact that line 77 can only get called when `if (!string.IsNullOrEmpty(fd.Name)).

So what is looks like is that AST was dropped by another task analyzing same module and it completed the analysis and removed the AST while this analysis was still running here. Alexander Sher (@AlexanderSher) - opinions?