-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use collection expression syntax for library import fixer when possible #97806
Use collection expression syntax for library import fixer when possible #97806
Conversation
Tagging subscribers to this area: @dotnet/interop-contrib Issue DetailsFixes: #96964 We prefer collection expression for C# 12 and above unless user explicitly disabled them via editorconfig rule
|
....Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ConvertToLibraryImportFixer.cs
Outdated
Show resolved
Hide resolved
....Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ConvertToLibraryImportFixer.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Aaron Robinson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @jkoritzinsky or @jtschuster ?
return true; | ||
} | ||
|
||
private static bool ShouldUseCollectionExpression(Document document, SyntaxTree syntaxTree) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing the SyntaxTree down, can we use document.GetOptionsAsync
and document.Project.ParseOptions
? This will require making more of these methods async
, but that's fine since eventually up the chain, the callers here are async already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was purposely avoiding adding async calls to minimize the diff. And document.GetOptionsAsync
returns DocumentOptionSet
, which is very different from AnalyzerOptions
I'm currently using. We can still avoid passing down SyntaxTree
if we get it in place with document.GetSyntaxTreeAsync()
, but that would require to propogate async
, so I'm not sure if this is actually a win. Can you point me to how DocumentOptionSet
works and how can I get the same result as from AnalyzerOptions
? Or do I need to use GetSyntaxTreeAsync
or just leave it as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking into Roslyn's APIs, they really don't make using well-known options any easier than custom options. We can keep this for now.
return true; | ||
} | ||
|
||
private static bool ShouldUseCollectionExpression(Document document, SyntaxTree syntaxTree) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking into Roslyn's APIs, they really don't make using well-known options any easier than custom options. We can keep this for now.
Fixes: #96964
We prefer collection expression for C# 12 and above unless user explicitly disabled them via editorconfig rule