Fixes #633 - Constructor names were classifying as types within xml docs #11651
Conversation
dpen2000
changed the title
|
@dotnet/roslyn-ide Please review. |
| @@ -137,8 +137,7 @@ private static bool IsNamespaceName(NameSyntax name) | |||
| if (symbol.Kind == SymbolKind.Alias) | |||
| { | |||
| symbol = (symbol as IAliasSymbol).Target; | |||
| } | |||
| else if (symbol.IsConstructor()) | |||
| } else if (symbol.IsConstructor() && name.Parent.Kind() == SyntaxKind.Attribute) | |||
There was a problem hiding this comment.
suggestion: you can use the IsParentKind() extension here
There was a problem hiding this comment.
Nit: else if should start on it's own line.
davkean
added
the
Community
|
Curious: this doesn't break just classification of "new Foo" in a regular context? |
|
@jasonmalinowski Doesn't seem to be: |
|
Huh. I guess the type we get in that case is a ITypeSymbol vs. the constructor? Then this looks good to me (other than the formatting nitpick). Any other takes from @dotnet/roslyn-ide? |
|
"Constructor names were classifying as types " Is that a bad thing? If you have: "new Foo()" then we classify that constructor as a type. |
|
@jasonmalinowski The Symbol in the "new MyClass" cases is NamedTypeSymbol. |
|
@CyrusNajmabadi This isn't "new Foo()" though: it's "Foo.Foo" so the second "foo" is definitely a constructor, rather than a type. |
|
@CyrusNajmabadi Also this classification as implemented here matches the classification in the tooltip as shown on the original bug |
|
Right. But i'm saying: why is it wrong to classify a constructor in this manner? :) I'm not getting why it's a bad thing. In general we treat constructors and types relatively interchangeably. For example, if you do a FindRefs on a type, we find references to the constructors as well. |
|
@CyrusNajmabadi I wouldn't say wrong. I'd say it's consistent with the colour of the name of constructor's identifier where defined and also with the display in QuickInfo. Even though it's impossible to nest a Foo class within a Foo class, it's still nice in this context where the constructor is being named to distinguish between a Nested Class and a Constructor by colour. |
|
LGTM with the formatting change Jason mentioned. |
|
That seems reasonable. Thanks for the PR! |
|
@jasonmalinowski @CyrusNajmabadi I've just done formatting change (and switching to IsParentKind as suggested by @balajikris). Thanks |
…ocumentation This is because NameSyntaxClassifier was choosing the parent symbol of a constructor to classify by. We're now restricting this to only in the case of attributes (which always refer to constructors but we want to classify like types).
|
👍 |
|
@dotnet-bot test vsi please |
|
@dotnet-bot retest windows_vsi_p2_open_prtest please |
|
@dotnet/roslyn-analysis : can someone from the team take a look at why this VSI test (CSharpErrorLog_v1_Engine.xml) is consistently failing on this PR with mismatched diagnostics count although the PR seemingly doesn't do anything related to diagnostics? thanks. |
|
@heejaechang can you please take a look here? |
|
@balajikris Any chance we can get someone to look at this? |
|
retest windows_vsi_p2_open_prtest please |
|
@dotnet/roslyn-infrastructure : can someone take this PR through to merging, after tests pass? It has the required sign offs. |
|
All tests passed, we have sufficient sign offs. Merging. |
|
Thanks for the contribution, @dpen2000 |
Fixes #633
This is because NameSyntaxClassifier was choosing the parent symbol of a constructor to classify by. We're now
restricting this to only in the case of attributes (which always refer to constructors but we want to classify like
types).
I'm not 100% sure if the behaviour of using the parent symbol in the case of constructors was just intended to cover attributes but restricting it to this satisfies the existing tests.
I first did the tests in SemanticClassifierTests but it's not clear which instance of "MyClass" is being referred to so I also did a test in TotalClassifierTests.
I've tested this in Visual Studio also and now the classification in the bug looks like this:
