Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add 7.1 support to UpgradeProject, add #version directive #17957

Closed
wants to merge 1 commit into from
Closed

Add 7.1 support to UpgradeProject, add #version directive #17957

wants to merge 1 commit into from

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Mar 18, 2017 •
edited
Loading

Adds #version (#17859)
Updates the UpgradeProject fixer as follow-up on #17894

@jcouv jcouv added this to the 15.3 milestone Mar 18, 2017
@jcouv jcouv self-assigned this Mar 18, 2017
@VSadov VSadov mentioned this pull request Mar 18, 2017
Closed
16 tasks
@jcouv
Copy link
Member Author

jcouv commented Mar 18, 2017

@gafter In VB, it was more natural to do #version "15" (following the format of #region directive).
In C#, I followed the model of the #error directive (parsing the rest of the line, so there are no quotes around the number in #version 7.1), but I'm wondering if I should change that to match VB instead.
Let me know what you think.

@gafter
Copy link
Member

gafter commented Mar 18, 2017

I think doing whatever is most natural is fine

@jcouv jcouv force-pushed the upgrade-project branch 3 times, most recently from e102079 to ca415e6 Compare March 19, 2017 03:53
@jcouv jcouv mentioned this pull request Mar 20, 2017
Merged
@jcouv
Copy link
Member Author

jcouv commented Mar 20, 2017

FYI @OmarTawfik This is not 100% ready, but fixes UpgradeProject to handle the new 7.1 error code.

@jcouv
Copy link
Member Author

jcouv commented Mar 20, 2017

Here's what this looks like...
In C#:

image

image

In VB:
image

image

@jcouv jcouv force-pushed the upgrade-project branch 2 times, most recently from aaef861 to a74bba0 Compare March 20, 2017 19:41
@jcouv
Copy link
Member Author

jcouv commented Mar 20, 2017

@dotnet/roslyn-compiler @dotnet/roslyn-ide for review.

The compiler change is to add a new #version directive. It always outputs an error diagnostic.
If it is followed by a version number (optional), then the error will be the same that CheckFeatureAvailability produces.
If it is left without a version number, then the error has the version of the compiler and the language version used for this compilation.
The purpose of #version is to make it easier to troubleshoot what language of the compiler and what language version settings people have. This came up with the 7.0 release, and will become more common with point releases.
It will also be useful to add a point release, as we usually do that before we have a feature that trigger the "please upgrade to version X" error.

The IDE changes are small: adding keyword completion for #version and updating the UpgradeProject code fixer to recognize C# 7.1 error trigger.

@jcouv jcouv force-pushed the upgrade-project branch from a74bba0 to 84b6460 Compare March 20, 2017 20:30
@jcouv jcouv assigned gafter and jcouv and unassigned jcouv Mar 20, 2017
@jcouv jcouv force-pushed the upgrade-project branch from 84b6460 to 60fbce2 Compare March 21, 2017 17:34
gafter
gafter reviewed Mar 21, 2017
View reviewed changes
src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt Outdated
@@ -153,6 +153,13 @@ Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax.WithCloseParenToken(Microso
Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax.WithElements(Microsoft.CodeAnalysis.SeparatedSyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax> elements) -> Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax
Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax.WithOpenParenToken(Microsoft.CodeAnalysis.SyntaxToken openParenToken) -> Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax
Microsoft.CodeAnalysis.CSharp.Syntax.VariableDesignationSyntax
Microsoft.CodeAnalysis.CSharp.Syntax.VersionDirectiveTriviaSyntax
Microsoft.CodeAnalysis.CSharp.Syntax.VersionDirectiveTriviaSyntax.Update(Microsoft.CodeAnalysis.SyntaxToken hashToken, Microsoft.CodeAnalysis.SyntaxToken versionKeyword, Microsoft.CodeAnalysis.SyntaxToken endOfDirectiveToken, bool isActive) -> Microsoft.CodeAnalysis.CSharp.Syntax.VersionDirectiveTriviaSyntax
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not require any API changes, I think. A #version 7.1 directive is already an error in the lexer, where all of the version information (language and assembly) is available. I think the implementation of this pseudo-feature should just be to change the error that is produced when that is recognized, and leave the syntax tree the same as it would have been previously, with only the diagnostic changed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gafter Tomas suggested that I use #error version as the trigger, for the main scenario.
And I will talk to the IDE team about achieving the second scenario with some trick in the IDE test layer instead.
Does that sound alright?

@jcouv jcouv force-pushed the upgrade-project branch from 60fbce2 to 3c4893f Compare March 22, 2017 02:20
@jcouv
Copy link
Member Author

jcouv commented Mar 22, 2017

I'll open another PR with different design.

@jcouv jcouv closed this Mar 22, 2017
@jcouv jcouv mentioned this pull request Mar 22, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gafter gafter gafter left review comments

Assignees

@gafter gafter

@jcouv jcouv

Labels
Area-Compilers cla-already-signed
Projects
None yet
Milestone
15.3
Development

Successfully merging this pull request may close these issues.
3 participants
@jcouv @gafter @dnfclas