Update SymbolDisplay API to include scoped for parameters and locals#64285
Update SymbolDisplay API to include scoped for parameters and locals#64285cston merged 11 commits intodotnet:mainfrom
Conversation
c9f79a4 to
ee0e0e7
Compare
| if (symbol.ScopedKind == ScopedKind.ScopedRef && | ||
| !symbol.IsThis && | ||
| format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.IncludeScoped)) | ||
| symbol.RefKind != RefKind.Out && |
There was a problem hiding this comment.
out parameters are implicitly scoped (at least from modules compiled with -langversion:11 or net7.0) so scoped is dropped from out parameters here, regardless of whether scoped appeared explicitly in source.
| !symbol.IsThis && | ||
| format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.IncludeScoped)) | ||
| symbol.RefKind != RefKind.Out && | ||
| !symbol.IsThis) |
There was a problem hiding this comment.
might want to comment these conditions to explain why they're there.
| { | ||
| AddKeyword(SyntaxKind.ScopedKeyword); | ||
| AddSpace(); | ||
| } |
There was a problem hiding this comment.
kinda surprised this isn't in AddParameterRefKindIfNeeded
There was a problem hiding this comment.
This scoped refers to the value rather than the ref. We could perhaps inline AddParameterRefKindIfNeeded() at the call-site above since it's only used in that one location. That might make the sequence clearer. But I've left AddParameterRefKindIfNeeded() as a distinct method for now, so the differences in this PR are easier to follow.
| AddKeyword(SyntaxKind.ReadOnlyKeyword); | ||
| AddKeyword(SyntaxKind.ScopedKeyword); | ||
| AddSpace(); | ||
| } |
There was a problem hiding this comment.
does this mean you can have ref scoped as well as scoped ref?
There was a problem hiding this comment.
does this mean you can have
ref scopedas well asscoped ref?
The ILocalSymbol API allows ref scoped but that is not supported from C#.
There was a problem hiding this comment.
Perhaps we should just make it into an else if so that ref and non-ref code paths do not overlap.
| AddKeyword(SyntaxKind.ScopedKeyword); | ||
| AddSpace(); | ||
| } | ||
| if (symbol.ScopedKind == ScopedKind.ScopedValue) |
There was a problem hiding this comment.
We could perhaps inline
AddParameterRefKindIfNeeded()at the call-site above since it's only used in that one location. That might make the sequence clearer.
I think we should seriously consider doing this and avoid introducing ref/non-ref overlapping code paths for scoped as we have right now. In other words, ScopedKind.ScopedValue should not affect ref parameters the same way as ScopedKind.ScopedRef doesn't affect value parameters. Language doesn't support these combinations and it will make the code easier to follow. BTW, I think it should be fine to do AddCustomModifiersIfNeeded(symbol.RefCustomModifiers ... only if we display ref, after all they are modifying the ref and will look very confusing without it. #Closed
There was a problem hiding this comment.
I've inlined AddParameterRefKindIfNeeded() but did not move AddCustomModifiers() into the if (IncludeModifiers) { } block. I agree it might be confusing to have ref custom modifiers without a leading ref but that seems like an unnecessary change for this PR.
There was a problem hiding this comment.
It looks like the code paths for ref and ScopedKind.ScopedValue still overlap. The primary motivation for suggesting refactoring was to avoid that.
AlekseyTs
left a comment
There was a problem hiding this comment.
LGTM (commit 8), with a refactoring suggestion.
Fixes #63208.