-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
UpgradeProject code fixer for C# #17435
Conversation
@@ -849,6 +849,7 @@ | |||
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.CompilerServer" /> | |||
<InternalsVisibleTo Include="VBCSCompiler" /> | |||
<InternalsVisibleTo Include="VBCSCompilerPortable" /> | |||
<InternalsVisibleTo Include="Microsoft.VisualStudio.LanguageServices" /> |
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.
This is not allowed.
ad1d0bc
to
f1cbc73
Compare
/// Displays the version number in the format expected on the command-line (/langver flag). | ||
/// For instance, "6", "7", "7.1", "latest". | ||
/// </summary> | ||
internal static string Display(this LanguageVersion version) |
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 would much like to avoid duplicating this method. Any tips? #Resolved
A couple of side notes:
|
Make it a public API. Or use some linked file trickery. #Resolved |
{ | ||
foreach (LanguageVersion version in Enum.GetValues(typeof(LanguageVersion))) | ||
{ | ||
yield return version; |
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.
Why the yield return when we're just doing a foreach to begin with? #Resolved
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.
Enum.GetValues
returns an Array
(msdn). #Resolved
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.
Can you not cast that array to LanguageVersion[]? #Resolved
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 think you can just cast that to a LanguageVersion[] (which is an IEnumerable) #Resolved
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.
or just call .Cast #Resolved
var fixAllProjectsTitle = string.Format(CSharpFeaturesResources.Upgrade_all_projects_to_csharp_language_version_0, | ||
newVersion.Display()); | ||
|
||
var fixAllProjects = new SolutionChangeAction(fixAllProjectsTitle, ct => UpgradeProjects(solution, newVersion)); |
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.
Why not using the cancellation token? #Resolved
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.
There's also a public API CodeAction.Create() rather than referencing the underlying type directly. #Resolved
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.
CodeAction.Create
produces warning RS0005: Do not use generic CodeAction.Create to create CodeAction
. #Resolved
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.
What do you think I should use the cancellation token in? There is no async code at this point (we just produce a callback for later). #Resolved
{ | ||
if (project.Language == LanguageNames.CSharp) | ||
{ | ||
currentSolution = await UpgradeProject(project, currentSolution, version).ConfigureAwait(true); |
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.
Why the asynchrony here when UpgradeProject is synchronous? #Resolved
{ | ||
if (project.Language == LanguageNames.CSharp) | ||
{ | ||
currentSolution = await UpgradeProject(project, currentSolution, version).ConfigureAwait(true); |
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.
This should be ConfigureAwait(false), not true, but no async is even better. #Resolved
} | ||
} | ||
|
||
private static Task<Solution> UpgradeProject(Project project, Solution solution, LanguageVersion version) |
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.
This is really confusing that you're passing both the project and solution, especially in the UpgradeProjects case where they are from different snapshots. #Resolved
@@ -501,6 +501,9 @@ Use the dropdown to view and switch to other projects this file may belong to.</ | |||
<data name="This_workspace_only_supports_opening_documents_on_the_UI_thread" xml:space="preserve"> | |||
<value>This workspace only supports opening documents on the UI thread.</value> | |||
</data> | |||
<data name="This_workspace_does_not_support_updating_VisualBasic_parse_options" xml:space="preserve"> | |||
<value>This workspace does not support updating VisualBasic parse options.</value> |
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.
"Visual Basic". (This also looks like we're really cutting corners here...) #Resolved
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'm happy to add support for VB, but I could not find a DTE API to update it. The intent is not to cut corners. #Resolved
@@ -217,6 +217,14 @@ | |||
<Project>{1EE8CAD3-55F9-4D91-96B2-084641DA9A6C}</Project> | |||
<Name>CodeAnalysis</Name> | |||
</ProjectReference> | |||
<ProjectReference Include="..\..\..\Compilers\CSharp\Portable\CSharpCodeAnalysis.csproj"> |
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.
Mentioned in another file, but this can't reference language specific binaries. #Resolved
@@ -305,6 +313,9 @@ | |||
<Reference Include="System.Xml.Linq" /> | |||
<Reference Include="UIAutomationProvider" /> | |||
<Reference Include="UIAutomationTypes" /> | |||
<Reference Include="VSLangProj2"> | |||
<HintPath>..\..\..\..\Binaries\VSLangProj2.dll</HintPath> |
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.
This seems fishy. #Resolved
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.
This will not be checked-in as-is. I'm blocked on getting a nuget package with this binary (so I copied it locally to unblock progress). #Resolved
@@ -1136,6 +1139,16 @@ private void CheckAllowedProjectChanges(ProjectChanges projectChanges) | |||
} | |||
} | |||
|
|||
private bool CanApplyParseOptionChange(ParseOptions old, ParseOptions @new) | |||
{ | |||
// only changes to C# LanguageVersion are supported at this point |
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.
This shouldn't be happening at the core Workspace type. The description you are defining is a behavior of the host, and should be overridable if necessary. #Resolved
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.
Hum. The trouble I have with moving this is that in VisualStudioWorkplaceImpl
I no longer am able to implement this check, because only the new ParseOptions
are available...
I'll need to think more about this.
@@ -25,6 +25,14 @@ | |||
<Project>{1EE8CAD3-55F9-4D91-96B2-084641DA9A6C}</Project> | |||
<Name>CodeAnalysis</Name> | |||
</ProjectReference> | |||
<ProjectReference Include="..\..\..\Compilers\CSharp\Portable\CSharpCodeAnalysis.csproj"> |
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.
This assembly also cannot reference language-specific types. #Resolved
The display function should very much be a public API. For the display, I'd recommend you look at the add reference code actions (@CyrusNajmabadi can point you to the exact code) because we have places today where we make other changes to projects and say what they are. #Resolved |
511a9ec
to
5d04a06
Compare
@@ -259,6 +260,8 @@ static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(this Mic | |||
static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(this Microsoft.CodeAnalysis.SemanticModel semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax declarationSyntax, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.ISymbol | |||
static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(this Microsoft.CodeAnalysis.SemanticModel semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.TupleExpressionSyntax declaratorSyntax, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.INamedTypeSymbol | |||
static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetForEachStatementInfo(this Microsoft.CodeAnalysis.SemanticModel semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.CommonForEachStatementSyntax forEachStatement) -> Microsoft.CodeAnalysis.CSharp.ForEachStatementInfo | |||
static Microsoft.CodeAnalysis.CSharp.LanguageVersionExtensions.Display(this Microsoft.CodeAnalysis.CSharp.LanguageVersion version) -> string | |||
static Microsoft.CodeAnalysis.CSharp.LanguageVersionExtensions.WithLanguageVersion(this Microsoft.CodeAnalysis.CSharp.LanguageVersion default, string new) -> Microsoft.CodeAnalysis.CSharp.LanguageVersion |
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.
FYI @gafter @jaredpar for public API change. We need an API that can print a language version to the format used on the command-line (for LanguageVersion.CSharp71
is should output "7.1") and we need an API for parsing back.
FYI @OmarTawfik since this is trying to make a public API exposing our parsing for language version. #Resolved
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.
The terminology we use for one direction is "ToDisplayString". I imagine we might want a TryParseDisplayString to go in the other direction. #Resolved
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'll change to ToDisplayString
.
For parsing, the trouble is which type should this method be exposed from? We can't have LanguageVersion.TryParseDisplayString
(which would have been ideal) because it is enum type :-(
return LanguageVersion.Default; | ||
case "latest": | ||
return LanguageVersion.Latest; | ||
default: |
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.
this worries me. #Resolved
var x = (1, 2); | ||
} | ||
}", | ||
new CSharpParseOptions(LanguageVersion.CSharp6)); |
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 don't see how this tets is actually validating anything. is the intent to add validation later? #Resolved
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 haven't figured out how to test more than this in unittest at this point. The test helper methods don't return anything (modified project or solution) that I could inspect.
I plan to add integration tests. #Resolved
new CSharpParseOptions(LanguageVersion.CSharp6)); | ||
} | ||
|
||
internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace) |
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.
goes at top of test file. #Resolved
@@ -482,6 +482,12 @@ | |||
<data name="Autoselect_disabled_due_to_possible_deconstruction_declaration" xml:space="preserve"> | |||
<value>Autoselect disabled due to possible deconstruction declaration.</value> | |||
</data> | |||
<data name="Upgrade_project_to_csharp_language_version_0" xml:space="preserve"> | |||
<value>Upgrade project to C# language version '{0}</value> |
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.
missing close quote. #Resolved
<value>Upgrade project to C# language version '{0}</value> | ||
</data> | ||
<data name="Upgrade_all_projects_to_csharp_language_version_0" xml:space="preserve"> | ||
<value>Upgrade all projects to C# language version '{0}</value> |
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.
missing close quote. #Resolved
var fixOneProjectTitle = string.Format(CSharpFeaturesResources.Upgrade_project_to_csharp_language_version_0, | ||
newVersion.Display()); | ||
|
||
var fixOneProject = new SolutionChangeAction(fixOneProjectTitle, |
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.
you'll need to provide your own subclass of SolutionChangeAction so that we can track these with telemetry. #Resolved
var fixOneProject = new SolutionChangeAction(fixOneProjectTitle, | ||
ct => Task.FromResult(UpgradeProject(solution, project.Id, newVersion))); | ||
|
||
var fixAllProjectsTitle = string.Format(CSharpFeaturesResources.Upgrade_all_projects_to_csharp_language_version_0, |
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.
only register this if the solution has multiple C# projects. #Resolved
newVersion.Display()); | ||
|
||
var fixAllProjects = new SolutionChangeAction(fixAllProjectsTitle, | ||
ct => Task.FromResult(UpgradeProjects(solution, newVersion))); |
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.
UpgradeAllProjects #Resolved
var fixAllProjects = new SolutionChangeAction(fixAllProjectsTitle, | ||
ct => Task.FromResult(UpgradeProjects(solution, newVersion))); | ||
|
||
return new CodeAction[] { fixOneProject, fixAllProjects }.AsImmutable(); |
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.
return ImmutableArray.Create(...) #Resolved
private const string CS8059 = nameof(CS8059); // error CS8059: Feature is not available in C# 6. Please use language version X or greater. | ||
|
||
public override ImmutableArray<string> FixableDiagnosticIds | ||
=> ImmutableArray.Create(CS8022, CS8023, CS8024, CS8025, CS8026, CS8059); |
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.
if you make this { get; } property, then you can just reference this property in a non-allocating way in your SuggestedVersion helper. #Resolved
{ | ||
internal abstract partial class AbstractUpgradeProjectCodeFixProvider : CodeFixProvider | ||
{ | ||
public override Task RegisterCodeFixesAsync(CodeFixContext context) |
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.
the core algorithm could be here (esp. as it will be the same for VB). Plus this is where you can put your SolutoinChangeAction subclass. Only keep C# specific logic in the C# layer. #Resolved
public string GetLanguageVersion(ParseOptions options) | ||
{ | ||
return ((CSharpParseOptions)options).LanguageVersion.Display(); | ||
} |
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.
use => #Resolved
{ | ||
return false; | ||
} | ||
var newLanguageVersion = parseOptionsService.GetLanguageVersion(@new); |
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.
newline after } #Resolved
namespace Microsoft.CodeAnalysis.CSharp | ||
{ | ||
[ExportLanguageService(typeof(IParseOptionsService), LanguageNames.CSharp), Shared] | ||
internal class CSharpParseOptionsService : IParseOptionsService |
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.
no VB impl? #Resolved
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.
The VB scenario cannot be lit up yet, because I couldn't find a DTE API to update the LanguageVersion
. Any tips how to handle that?
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.
@jcouv: file a bug if you haven't.
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.
Tracked by #17173
…o IParseOptionsService
@dotnet-bot test ubuntu_14_debug_prtest please |
@dotnet-bot test windows_release_vs-integration_prtest please |
@dotnet-bot test windows_debug_vs-integration_prtest please |
1 similar comment
@dotnet-bot test windows_debug_vs-integration_prtest please |
Yay! Merged :-) |
Some background:
Starting with the first quarterly release of VS2017, we plan to release minor versions of the languages.
This means two changes to the compiler:
This will be accompanied by some IDE changes:
While it is always possible for users to select a specific language version, we expect most users to use "default" (slow train) or "latest" (fast train).
This code fixer will assist users who are on "default" to upgrade their project configuration to "latest".
Relates to #17173 (adding support for point releases)
@dotnet/roslyn-ide for discussion and review.
FYI @gafter @jaredpar