Repository navigation
VB generators - #49139
VB generators#49139
Conversation
- Add GetGeneratorsForAllLanguages - Don't return duplicates for generators
| } | ||
|
|
||
| [Obsolete("Use GetGenerators(string language) or GetGeneratorsForAllLanguages()")] | ||
| public override ImmutableArray<ISourceGenerator> GetGenerators() |
There was a problem hiding this comment.
📝@333fred I'd like to run this by the API compat review board.
There was a problem hiding this comment.
Is your hope to just break it? Does this fall under our "source generator hosting isn't frozen" bar?
There was a problem hiding this comment.
In a perfect world, I'd just remove it. Right now I've obsoleted it, and it returns only CSharp generators, which is essentially what it did before.
| return _diagnosticAnalyzers.GetExtensionsForAllLanguages(); | ||
| // This API returns duplicates of analyzers that support multiple languages. | ||
| // We explicitly retain this behaviour to ensure back compat | ||
| return _diagnosticAnalyzers.GetExtensionsForAllLanguages(includeDuplicates: true); |
There was a problem hiding this comment.
📝I'm not sure if it was ever intentional to return duplicate analyzer references, but I've ensured we keep the previous behavior.
There was a problem hiding this comment.
Should we just break this? I admit I can't think of anything that would care, and honestly more that might be broken. :-)
There was a problem hiding this comment.
Yeah, I have no idea. It is a public API though, and it's been around for at least 6 years according to blame.
| public sealed class GeneratorAttribute : Attribute | ||
| { | ||
| // https://github.com/dotnet/roslyn/issues/: we don't know if we'll keep this, but for now it lets us re-use the analyzer discovery mechanism | ||
| /// <summary> |
There was a problem hiding this comment.
❓ This is the same layout as the DiagnosticAnalyzer attribute, so that we can re-use the lookup logic. Should we enforce this via some stronger mechanism like inheritance (does that break binary compat?) or are we confident with tests covering it working as expected?
There was a problem hiding this comment.
I'm fine with it being separate. I think inheritance may be over doing it a bit here.
|
@dotnet/roslyn-compiler for review (/cc @jasonmalinowski for IDE). Hopefully it's fairly clear what's going on, but you may find it easier to review commit-by-commit, as I tried to do the work in small distinct stages to make review easier. |
|
|
||
| internal AdditionalSourcesCollection() | ||
| internal AdditionalSourcesCollection(string fileExtension) | ||
| { |
There was a problem hiding this comment.
When dealing with extensions can never remember if we should or should not include the .. So ".cs" vs. "cs". Consider either enforcing one or the other is passed here or normalizing the input to a specific one.
| public string[] Languages { get; } | ||
|
|
||
| /// <summary> | ||
| /// Attribute constructor used to specify the attached class is a source generator that provides CSharp sources. |
There was a problem hiding this comment.
| /// Attribute constructor used to specify the attached class is a source generator that provides CSharp sources. | |
| /// Attribute constructor used to specify the attached class is a source generator that provides CSharp sources. |
Crazy aside: is it required to be a class or can it say be a struct?
There was a problem hiding this comment.
<insert appropriate meme here for @jaredpar asking crazy questions>
More seriously, when generators are ran the IDE needs to compare results from one run to the next. I naively used == to compare the instances, so...
There was a problem hiding this comment.
It has to be a class. We explicitly set the allowed location of the attribute to be class (see line 11) so loading via AnalyzerReference it can't ever be a struct.
As a second defense we check that the generator is also an ISourceGenerator, so there's no way you can pass a struct via the generator driver in code either.
| return peModule.IsTargetAttribute(customAttrHandle, s_generatorAttributeNamespace, nameof(GeneratorAttribute), ctor: out _); | ||
| } | ||
|
|
||
| private static IEnumerable<string> GetGeneratorSupportedLanguages(PEModule peModule, CustomAttributeHandle customAttrHandle) |
There was a problem hiding this comment.
Could this be a local function?
There was a problem hiding this comment.
I've left these for now as they've got the comments explaining them and it seemed weird embedding them in the ctor.
| return ReadLanguagesFromAttribute(ref argsReader); | ||
| } | ||
|
|
||
| private static bool IsGeneratorAttribute(PEModule peModule, CustomAttributeHandle customAttrHandle) |
There was a problem hiding this comment.
Could this be a local function?
There was a problem hiding this comment.
I refactored out the extensions logic to make it simpler and not need it.
|
Closing/reopening to fix CI |
jasonmalinowski
left a comment
jasonmalinowski
left a comment
There was a problem hiding this comment.
IDE stuff looks good except it also needs a change in src/Features/Core/Portable/Diagnostics/AnalyzerHelper.cs, where we updated in #48400. There's comments with "we don't support VB" that need to be updated with the right diagnostic IDs.
| } | ||
|
|
||
| [Obsolete("Use GetGenerators(string language) or GetGeneratorsForAllLanguages()")] | ||
| public override ImmutableArray<ISourceGenerator> GetGenerators() |
There was a problem hiding this comment.
Is your hope to just break it? Does this fall under our "source generator hosting isn't frozen" bar?
| public string[] Languages { get; } | ||
|
|
||
| /// <summary> | ||
| /// Attribute constructor used to specify the attached class is a source generator that provides CSharp sources. |
There was a problem hiding this comment.
<insert appropriate meme here for @jaredpar asking crazy questions>
More seriously, when generators are ran the IDE needs to compare results from one run to the next. I naively used == to compare the instances, so...
|
Is this internal or the same as the C# Source Generator feature or something else? If its public is there any VB specific documentation/examples? |
|
@jasonmalinowski I've updated the AnalyzerHelper to include the VB warning, and added the required warning to the VB code itself. I have a test in the VB layer, but currently can't run it because we're not running VB command line tests on core. That is proving to be an interesting challenge, so I may just open an issue for it and add the test as part of the move later on. |
|
Ok I've added the test, even though it will always be skipped for now, and opened an issue to track enabling the command-line tests for VB. |
|
When does this appear in some Preview I can try? |
When the PR merges it'll be available in the Roslyn nightly NuGet packages. We don't have a definitive date or target release of visual studio that it'll be available by, but we'll be sure to call it out when there is one. |
|
@chsienki when it is available in NuGet can you post minimal instructions on where to download and simple usage? |
|
@dotnet/roslyn-compiler for a second review please :) |
| return _diagnosticAnalyzers.GetExtensionsForAllLanguages(); | ||
| // This API returns duplicates of analyzers that support multiple languages. | ||
| // We explicitly retain this behaviour to ensure back compat | ||
| return _diagnosticAnalyzers.GetExtensionsForAllLanguages(includeDuplicates: true); |
There was a problem hiding this comment.
Should we just break this? I admit I can't think of anything that would care, and honestly more that might be broken. :-)
ghost
added this to the
7 participants
Enable generators in the Visual Basic compiler