File types IDE changes by RikkiGibson · Pull Request #62215 · dotnet/roslyn

RikkiGibson · 2022-06-28T22:29:50Z

Related to #60819

@dibarbet for review. This gets us to the min-bar for IDE support.

The SymbolKey changes are important here. Without them EnC will not work at all with the feature. With them we can do some simple EnC without crashing in the IDE. It will still fail when files are added or removed during an EnC session but we anticipate solving that after merging the feature to main, before RTM. #61999

RikkiGibson · 2022-06-28T22:34:11Z

Just realized this won't pass CI until #61646 is merged 😅. Should still be reviewable though.

...kspaces/SharedUtilitiesAndExtensions/Compiler/Core/SymbolKey/SymbolKey.NamedTypeSymbolKey.cs

Add helper

…ft-ide

src/EditorFeatures/CSharpTest/SymbolKey/SymbolKeyTests.cs

src/EditorFeatures/CSharpTest/SymbolKey/SymbolKeyCompilationsTests.cs

src/EditorFeatures/CSharpTest/Completion/CompletionProviders/KeywordCompletionProviderTests.cs

Youssef1313 · 2022-07-01T03:23:25Z

src/Workspaces/Core/Portable/Editing/DeclarationModifiers.cs

            Volatile    = 1 << 14,
            Extern      = 1 << 15,
            Required    = 1 << 16,
+            File        = 1 << 17,


Since you added this API, consider fixing up:

Consider also updating

roslyn/src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs

Lines 1491 to 1492 in 5510ef9

case SyntaxKind.DelegateDeclaration:

return DeclarationModifiers.New | DeclarationModifiers.Unsafe;

roslyn/src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs

Lines 1444 to 1466 in 5510ef9

private static readonly DeclarationModifiers s_classModifiers =

DeclarationModifiers.Abstract |

DeclarationModifiers.New |

DeclarationModifiers.Partial |

DeclarationModifiers.Sealed |

DeclarationModifiers.Static |

DeclarationModifiers.Unsafe;

private static readonly DeclarationModifiers s_recordModifiers =

DeclarationModifiers.Abstract |

DeclarationModifiers.New |

DeclarationModifiers.Partial |

DeclarationModifiers.Sealed |

DeclarationModifiers.Unsafe;

private static readonly DeclarationModifiers s_structModifiers =

DeclarationModifiers.New |

DeclarationModifiers.Partial |

DeclarationModifiers.ReadOnly |

DeclarationModifiers.Ref |

DeclarationModifiers.Unsafe;

private static readonly DeclarationModifiers s_interfaceModifiers = DeclarationModifiers.New | DeclarationModifiers.Partial | DeclarationModifiers.Unsafe;

It actually wasn't clear to me whether 'file' should be added to this. I assume if the IDE has need to generate syntax for a file type, it would use this. But I don't know if the use case exists. It would be helpful to know whether I should delete and just stub out a bool IsFIle => false; implementation.

@RikkiGibson This is a public API. So it's very necessary that 'file' is added here (not necessarily in this PR)

A common SyntaxGenerator usage pattern is generator.WithModifiers(node, generator.GetModifiers(node).WithIsX(true))

If the node with a class declaration for file class C { }, and we called WithIsSealed(true) (for example), I expect the resulting node will drop file if the change is not made.

@Youssef1313 is correct. thanks! :)

src/Workspaces/Core/Portable/Editing/DeclarationModifiers.cs

jcouv · 2022-07-01T16:42:46Z

src/Compilers/Core/Portable/Symbols/INamedTypeSymbol.cs

+        /// <summary>
+        /// Indicates the type is declared in source and is only visible in the file it is declared in.
+        /// </summary>
+        bool IsFile { get; }


📝 For the record, the API review for this is tracked by #62254

src/Compilers/Core/Portable/Symbols/INamedTypeSymbol.cs

jcouv

Done with review pass, compiler changes only (iteration 14)

Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>

…t-ide

CyrusNajmabadi

lgtm with the changes suggested by @Youssef1313 . thanks!

RikkiGibson · 2022-07-01T20:34:01Z

src/EditorFeatures/Test/CodeGeneration/CodeGenerationTests.CSharp.cs

+            [InlineData("struct", TypeKind.Struct)]
+            [InlineData("interface", TypeKind.Interface)]
+            [InlineData("enum", TypeKind.Enum)]
+            public async Task AddFileType(string kindString, TypeKind typeKind)


I wasn't able to figure out how to properly test 'delegate', 'record', or 'record struct' here.

'delegate' got me a test crash with a stacktrace consisting only of async thunks..

I didn't see how to specify 'record' and 'record struct' in here.

Youssef1313

I expect the lack of file being handled here to cause problems with a public API path:

roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpAccessibilityFacts.cs

Lines 126 to 143 in 1fcfda5

    
           modifiers |= token.Kind() switch 
        
           { 
        
               SyntaxKind.AbstractKeyword => DeclarationModifiers.Abstract, 
        
               SyntaxKind.NewKeyword => DeclarationModifiers.New, 
        
               SyntaxKind.OverrideKeyword => DeclarationModifiers.Override, 
        
               SyntaxKind.VirtualKeyword => DeclarationModifiers.Virtual, 
        
               SyntaxKind.StaticKeyword => DeclarationModifiers.Static, 
        
               SyntaxKind.AsyncKeyword => DeclarationModifiers.Async, 
        
               SyntaxKind.ConstKeyword => DeclarationModifiers.Const, 
        
               SyntaxKind.ReadOnlyKeyword => DeclarationModifiers.ReadOnly, 
        
               SyntaxKind.SealedKeyword => DeclarationModifiers.Sealed, 
        
               SyntaxKind.UnsafeKeyword => DeclarationModifiers.Unsafe, 
        
               SyntaxKind.PartialKeyword => DeclarationModifiers.Partial, 
        
               SyntaxKind.RefKeyword => DeclarationModifiers.Ref, 
        
               SyntaxKind.VolatileKeyword => DeclarationModifiers.Volatile, 
        
               SyntaxKind.ExternKeyword => DeclarationModifiers.Extern, 
        
               _ => DeclarationModifiers.None, 
        
           };

RikkiGibson · 2022-07-01T20:42:29Z

Are you able to identify a specific editor feature/scenario which will behave incorrectly? I may prefer to file an issue
and address it after this PR.

Youssef1313 · 2022-07-01T20:43:16Z

Are you able to identify a specific editor feature/scenario which will behave incorrectly? I may prefer to file an issue and address it after this PR.

I'll take a look

jcouv

Compiler-side LGTM Thanks (iteration 20)

RikkiGibson added 2 commits June 28, 2022 15:24

Recommend file keyword

6a088fe

Fix SymbolKey for file types

107bf5e

RikkiGibson requested a review from a team as a code owner June 28, 2022 22:29

ghost added the Area-IDE label Jun 28, 2022

CyrusNajmabadi reviewed Jun 28, 2022

View reviewed changes

...kspaces/SharedUtilitiesAndExtensions/Compiler/Core/SymbolKey/SymbolKey.NamedTypeSymbolKey.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 28, 2022

View reviewed changes

...kspaces/SharedUtilitiesAndExtensions/Compiler/Core/SymbolKey/SymbolKey.NamedTypeSymbolKey.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 4 commits June 28, 2022 16:27

Simplify symbol key creation/resolution for named types

75fe197

Add helper

Add support for file types

8ee1089

Add tests

d889b35

Rev the version

b547e91

RikkiGibson mentioned this pull request Jun 29, 2022

File types emit #61646

Merged

RikkiGibson added 3 commits June 29, 2022 17:37

Revert changes to NamedTypeSymbolKey

0ed1fb8

Merge remote-tracking branch 'CyrusNajmabadi/fileTypeSymbolKey' into …

0ee88d9

…ft-ide

Merge remote-tracking branch 'upstream/features/file-types' into ft-ide

80a7e62

Youssef1313 reviewed Jun 30, 2022

View reviewed changes

src/EditorFeatures/CSharpTest/SymbolKey/SymbolKeyTests.cs Show resolved Hide resolved

Implement INamedTypeSymbol.IsFile. Fix tests.

6452012

RikkiGibson requested a review from a team as a code owner June 30, 2022 22:55

RikkiGibson added 2 commits June 30, 2022 15:58

Add suggested test

6c0dd3a

Fix metadata name

6465744

cston reviewed Jun 30, 2022

View reviewed changes

src/EditorFeatures/CSharpTest/SymbolKey/SymbolKeyCompilationsTests.cs Show resolved Hide resolved

cston reviewed Jun 30, 2022

View reviewed changes

src/EditorFeatures/CSharpTest/SymbolKey/SymbolKeyCompilationsTests.cs Show resolved Hide resolved

cston approved these changes Jun 30, 2022

View reviewed changes

Verify number of symbols found

5c20db8

RikkiGibson requested review from CyrusNajmabadi, dibarbet and jcouv July 1, 2022 00:35

davidwengier reviewed Jul 1, 2022

View reviewed changes

src/EditorFeatures/CSharpTest/Completion/CompletionProviders/KeywordCompletionProviderTests.cs Show resolved Hide resolved

cston approved these changes Jul 1, 2022

View reviewed changes

fix expected lengths

200bf26

Youssef1313 reviewed Jul 1, 2022

View reviewed changes

src/Workspaces/Core/Portable/Editing/DeclarationModifiers.cs Outdated Show resolved Hide resolved

jcouv added the Area-Compilers label Jul 1, 2022

jcouv reviewed Jul 1, 2022

View reviewed changes

src/Compilers/Core/Portable/Symbols/INamedTypeSymbol.cs Show resolved Hide resolved

jcouv reviewed Jul 1, 2022

View reviewed changes

RikkiGibson and others added 3 commits July 1, 2022 10:34

Add compiler tests for new public API

6f1e7ef

Add suggested tests

d337467

Update src/Workspaces/Core/Portable/Editing/DeclarationModifiers.cs

603f1c2

Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>

cston approved these changes Jul 1, 2022

View reviewed changes

RikkiGibson added 2 commits July 1, 2022 12:47

Merge branch 'features/file-types' of github.com:dotnet/roslyn into f…

b724def

…t-ide

Merge branch 'ft-ide' of github.com:RikkiGibson/roslyn into ft-ide

dbebadb

CyrusNajmabadi approved these changes Jul 1, 2022

View reviewed changes

Add type generation tests

d9706eb

RikkiGibson requested review from a team and CyrusNajmabadi and removed request for CyrusNajmabadi July 1, 2022 20:32

RikkiGibson commented Jul 1, 2022

View reviewed changes

Youssef1313 reviewed Jul 1, 2022

View reviewed changes

RikkiGibson requested a review from jcouv July 1, 2022 23:48

jcouv self-assigned this Jul 5, 2022

jcouv approved these changes Jul 5, 2022

View reviewed changes

RikkiGibson merged commit 53b193b into dotnet:features/file-types Jul 5, 2022

RikkiGibson deleted the ft-ide branch July 5, 2022 14:15

This was referenced Jul 5, 2022

File and required modifiers are missing from CSharpAccessibilityFacts.GetAccessibilityAndModifiers #62371

Closed

Add support for file-types in SymbolKey. #62218

Closed

ryzngard mentioned this pull request Jul 27, 2022

Enable new inline rename by default #62992

Merged

jcouv mentioned this pull request Oct 7, 2022

Test plan for "file types" #60819

Closed

83 tasks

	case SyntaxKind.DelegateDeclaration:
	return DeclarationModifiers.New \| DeclarationModifiers.Unsafe;

	private static readonly DeclarationModifiers s_classModifiers =
	DeclarationModifiers.Abstract \|
	DeclarationModifiers.New \|
	DeclarationModifiers.Partial \|
	DeclarationModifiers.Sealed \|
	DeclarationModifiers.Static \|
	DeclarationModifiers.Unsafe;

	private static readonly DeclarationModifiers s_recordModifiers =
	DeclarationModifiers.Abstract \|
	DeclarationModifiers.New \|
	DeclarationModifiers.Partial \|
	DeclarationModifiers.Sealed \|
	DeclarationModifiers.Unsafe;

	private static readonly DeclarationModifiers s_structModifiers =
	DeclarationModifiers.New \|
	DeclarationModifiers.Partial \|
	DeclarationModifiers.ReadOnly \|
	DeclarationModifiers.Ref \|
	DeclarationModifiers.Unsafe;

	private static readonly DeclarationModifiers s_interfaceModifiers = DeclarationModifiers.New \| DeclarationModifiers.Partial \| DeclarationModifiers.Unsafe;

	modifiers \|= token.Kind() switch
	{
	SyntaxKind.AbstractKeyword => DeclarationModifiers.Abstract,
	SyntaxKind.NewKeyword => DeclarationModifiers.New,
	SyntaxKind.OverrideKeyword => DeclarationModifiers.Override,
	SyntaxKind.VirtualKeyword => DeclarationModifiers.Virtual,
	SyntaxKind.StaticKeyword => DeclarationModifiers.Static,
	SyntaxKind.AsyncKeyword => DeclarationModifiers.Async,
	SyntaxKind.ConstKeyword => DeclarationModifiers.Const,
	SyntaxKind.ReadOnlyKeyword => DeclarationModifiers.ReadOnly,
	SyntaxKind.SealedKeyword => DeclarationModifiers.Sealed,
	SyntaxKind.UnsafeKeyword => DeclarationModifiers.Unsafe,
	SyntaxKind.PartialKeyword => DeclarationModifiers.Partial,
	SyntaxKind.RefKeyword => DeclarationModifiers.Ref,
	SyntaxKind.VolatileKeyword => DeclarationModifiers.Volatile,
	SyntaxKind.ExternKeyword => DeclarationModifiers.Extern,
	_ => DeclarationModifiers.None,
	};

Conversation

RikkiGibson commented Jun 28, 2022

Uh oh!

RikkiGibson commented Jun 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Youssef1313 Jul 1, 2022

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jul 1, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jul 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcouv Jul 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jul 1, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Youssef1313 commented Jul 1, 2022

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Youssef1313 Jul 1, 2022 •

edited

Loading

Youssef1313 left a comment •

edited

Loading

RikkiGibson commented Jul 1, 2022 •

edited

Loading