Complete 'file' support for SyntaxGenerator
#62413
Conversation
ghost
added
the
Community
|
@RikkiGibson @CyrusNajmabadi @sharwell I'm not sure about the expected behavior for some cases. The current behavior aligns with an existing behavior for static constructors, but the behavior is debatable, and I'm unsure if we should just keep consistent with static constructors, or have different behavior, or even change the behavior of static constructors. |
| [Fact] | ||
| public void TestAddFileModifierToPublicClass() | ||
| { | ||
| var publicClass = (ClassDeclarationSyntax)SyntaxFactory.ParseMemberDeclaration("public class C { }"); | ||
| var filePublicClass = Generator.WithModifiers(publicClass, Generator.GetModifiers(publicClass).WithIsFile(true)); | ||
| // bad output. It will be fixed in a later commit. | ||
| // This commit just demonstrates the *current* behavior (test is passing currently, while it shouldn't). | ||
| // note, behavior changed here in this commit since we supported file, but the most correct behavior is to eliminate public completely. | ||
| // note 2, the correct behavior here is actually not very clear given the constructor case and the opposite case of this. | ||
| VerifySyntax<ClassDeclarationSyntax>(filePublicClass, @"public file class C | ||
| { | ||
| }"); | ||
| } |
There was a problem hiding this comment.
This is even more confusing for a case where the class originally didn't have any accessibility or modifiers (ie, a plain class C), and WithModifiers is called with DeclarationModifiers.Public | DeclarationModifiers.File. Yes it's weird that someone do this, but.. it's a public API.
There was a problem hiding this comment.
i'm ok with this just throwing away public modifiers in that case. we did similar things with interface members (prior to them supporting accessibility or method bodies).
There was a problem hiding this comment.
This is actually my bad. There is no DeclarationModifiers.Public 😄
The way to say both public and file will be two calls, one to WithModifiers and one to WithAccessibility. The second call will overwrite the first in this case.
| @@ -1628,6 +1628,9 @@ private static SyntaxTokenList AsModifierList(Accessibility accessibility, Decla | |||
| break; | |||
| } | |||
|
|
|||
| if (modifiers.IsFile) | |||
| list.Add(SyntaxFactory.Token(SyntaxKind.FileKeyword)); | |||
There was a problem hiding this comment.
if we have .IsFile can we not add any accessibility above? one of the goals of SyntaxGenerator is to add smarts like that to generate the most likely thing wanted.
|
@CyrusNajmabadi This is ready for another look |
|
Thanks @Youssef1313! |
* upstream/main: (62 commits) Prevent assert from being hit (dotnet#62366) Don't offer '??=' for pointer types (dotnet#62476) Integrate generator times into /reportAnalyzer (dotnet#61661) Switch to a callback for cleaning up resources instead of passing in an explicit IDisposable. (dotnet#62373) Filter trees to only those containing global-usings or attributes prior to analyzing them. (dotnet#62444) Update PublishData.json Complete 'file' support for `SyntaxGenerator` (dotnet#62413) Apply changes directly to text buffer (dotnet#62337) Remove LangVer check from extended nameof binding (dotnet#62339) Fixed shared project file error (dotnet#62466) Handle new error codes Use MSBuid generated property for package path Exclude BCL libraries from Roslyn vsix Bump the integration test timeouts a bit Skip the balanced switch dispatch optimization for patterns on floating-point inputs (dotnet#62322) Test helpers shouldn't escape quotes by default (dotnet#62321) Reuse prior TableEntry values in the increment NodeTable builder if possible. (dotnet#62320) Install 3.1 runtime for SBOM tool Generate VS SBOM during official build. Minor refactoring in 'required' handling for override completion (dotnet#62422) ...
Part of #62371 (pending
requiredtests and support, will do in a follow-up)Fixes #62274
Review should be commit-by-commit.