Annotate remaining symbol interfaces#39184
Conversation
2bfca77 to
5b81932
Compare
src/Workspaces/Core/Portable/Shared/Extensions/ISymbolExtensions.cs
Outdated
Show resolved
Hide resolved
5b81932 to
295cc81
Compare
nit: I think this API technically allows a Refers to: src/Compilers/Core/Portable/Symbols/INamespaceOrTypeSymbol.cs:32 in 295cc81. [](commit_id = 295cc81, deletion_comment = False) |
jcouv
left a comment
There was a problem hiding this comment.
LGTM Thanks (iteration 1) with some nits to consider
I'm leaving |
| private static bool HasBackingField(IEventSymbol @event) | ||
| { | ||
| #nullable disable // https://github.com/dotnet/roslyn/issues/39288 | ||
| return @event.AddMethod.IsImplicitlyDeclared |
There was a problem hiding this comment.
The doc on AddMethod says "Null only in error scenarios.". The EnC analyzer does not perform semantic analysis in documents that have syntax errors. It would be useful to clarify what "error scenarios" means - is it syntax errors in the event declaration or could it also be a semantic error? I'd think that the symbol will be non-null in presence of semantic errors (error symbols might be present in the method symbol signature, but it won't be null).
There was a problem hiding this comment.
I'm going to leave this for a follow-up change per #39288
There was a problem hiding this comment.
The only time I'm aware of it happening is when the event has an accessor list that is missing an accessor. I think it's not a syntax error because it's not contained in SyntaxTree.GetDiagnostics() for the tree containing the bad declaration. It is reported when we are creating the source event symbol.
There was a problem hiding this comment.
I see. Seems like we may have a bug here.
This pull request includes interfaces except for
ISymbolitself.