Relax ordering constraints around 'ref' and 'partial' modifiers #23533

alrz · 2017-12-02T21:42:54Z

Proposal: dotnet/csharplang#946 (as part of C# 7.3)

alrz · 2017-12-04T22:04:32Z

requested a review from dotnet/roslyn-ide as a code owner 2 days ago

That's weird. This doesn't touch any ide code. (maybe we should add root slashes to paths?)

/cc @jasonmalinowski This is for the compiler team to review. Thanks.
#Resolved

jasonmalinowski · 2017-12-04T22:10:28Z

@alrz: I see you rebased the branch. If you branched master, retargeted to the PR to features/compiler, but hadn't rebased your branch first, then you might have in that window of time carried along an IDE fix which got that added.

Or you found a bug in GitHub. We'll see if we see it again? #Resolved

alrz · 2017-12-04T22:15:01Z

@jasonmalinowski

Also, by clicking on "code owner" in "requested a review from dotnet/roslyn-ide as a code owner" line you will be redirected to the line that caused it. it's on master. sounds like a GitHub bug. #Resolved

jasonmalinowski · 2017-12-04T22:35:19Z

@alrz Yeah, did you change GitHub branch target and then do a rebase? Yeah GitHub shows it's the EditorFeatures line, but unfortunately doesn't let us see the point-in-time diff it was looking at when it decided to do that. #Resolved

alrz · 2017-12-05T21:45:28Z

"Relax ordering constraints around ref and partial modifiers on type declarations" (possibly will be treated as a 7.2 bug fix)

@gafter should I rebase to master? #Resolved

gafter · 2017-12-05T22:02:17Z

@alrz We'll let you know

@jcouv @jaredpar FYI this is a community contribution for "Relax ordering constraints around 'ref' and 'partial' modifiers on type declarations" #Resolved

alrz · 2017-12-07T19:50:17Z

IMO this should be behind a feature flag if we intend to relax partial ordering as well (this is implemented in this PR). there's another fix for dotnet/csharplang#1022 in #23643 (per LDM 2017-12-04)

We allow ref this but not the other way around. That's wrong! We can't disallow ref this, but we should allow and prefer this ref.

which I think could be a bug fix. #Resolved

agocke · 2017-12-08T18:56:36Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

-                        var isPartialType = this.IsPartialType();
-                        var isPartialMember = this.IsPartialMember();
-                        if (isPartialType || isPartialMember)
+                        var isPartialFollowedByType = this.IsPartialType();


I think we should change or remove the IsPartialType helper. It no longer does what it says it does.

We should also add tests for the case described in the comment for ParseIdentifierToken

agocke · 2017-12-08T18:59:32Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

-                        var isPartialMember = this.IsPartialMember();
-                        if (isPartialType || isPartialMember)
+                        var isPartialFollowedByType = this.IsPartialType();
+                        var isPartialType = isPartialFollowedByType || IsNonContextualModifier(nextToken);


What's the result if we try to parse partial partial ref struct? #Resolved

I think the diagnostic will be similar to what we have today (expected ; etc). I'll add the test.

agocke · 2017-12-08T19:00:22Z

src/Compilers/CSharp/Test/Syntax/Parsing/ParserErrorMessageTests.cs

-                // (2,1): error CS0267: The 'partial' modifier can only appear immediately before 'class', 'struct', 'interface', or 'void'
-                // partial public class C  // CS0267
-                Diagnostic(ErrorCode.ERR_PartialMisplaced, "partial").WithLocation(2, 1));
+            CreateStandardCompilation(test).VerifyDiagnostics();


In all of the cases where compilation now succeeds, I think we should access the type symbol for the declaration from the Compilation and verify that all the modifiers we expect are now present on the type.

alrz · 2017-12-09T04:25:31Z

the update xlf thing is such a drag. now for every resx change we need to commit 15 files? I thought we were going to narrow it down to one (just resx, not even .designer.cs) #Resolved

sharwell · 2017-12-11T13:42:51Z

the update xlf thing is such a drag. now for every resx change we need to commit 15 files? I thought we were going to narrow it down to one (just resx, not even .designer.cs)

/cc @tmeschter @nguerrera #Resolved

tmeschter · 2017-12-11T17:14:03Z

@alrz We're in the process of moving localization work into the dotnet/roslyn repository. The upside is that we will, eventually, be able to take community contributions to improve translations. The downside is the need to keep the .xlf files up-to-date with the .resx files to ensure our translators are not working with stale data. We're looking into ways of automating this so you won't have to deal with it in normal developer workflows.

Note that changes to .resx and .designer.cs will generally have to happen together for correctness reasons, but that's always how it has been. #Resolved

AlekseyTs · 2017-12-11T19:18:56Z

@alrz Has this been approved by LDM? #Resolved

jcouv · 2017-12-11T19:44:12Z

@AlekseyTs The last notes on this only recorded that we want to relax for ref, but from my recollection and Andy's there was later agreement to relax for partial.
Mads should be in office some time next week. I'll get confirmation from him then. #Resolved

alrz · 2017-12-14T10:48:03Z

@tmeschter

Note that changes to .resx and .designer.cs will generally have to happen together for correctness reasons, but that's always how it has been.

I think we can actually exclude .designer.cs from worktree just like .g.i.cs for xaml files.
#Resolved

tmeschter · 2017-12-14T17:47:39Z

I think we can actually exclude .designer.cs from worktree just like .g.i.cs for xaml files.

No, that's not safe. The .designer.cs file is updated by VS as the .resx files changes; it is not updated during the build. If you don't commit and flow changes to the .designer.cs along with the .resx then you'll quickly run into problems where your changes build on your local system, but don't build on a CI system. #Resolved

tmeschter · 2017-12-14T17:50:46Z

The conflicts in the .xlf files can be dealt with as follows:

Resolve the conflicts in the .resx file as you would normally.
Bulk resolve all conflicts in the .xlf files in favor of the other branch (this preserves all updated translations coming from the other branch).
Build to bring the .xlf files in sync with the .resx files again.
Commit all .xlf files updated in step 3. #Resolved

nguerrera · 2017-12-14T18:41:44Z

This is how to do those steps on the command line if you don't use GUI tools to handle bulk conflict resolution.

# merge and get conflicts (assuming upstream remote name and using base branch of this particular PR)
git fetch --all
git merge upstream/features/compiler

# overwrite xlf with their changes
git checkout --theirs '**/*.xlf'

# build to get your xlf updates reapplied
build

# stage updated xlf and mark conflict resolved
git add '**/*.xlf'

# commit merge
git commit
``` #Resolved

jcouv · 2019-09-12T17:52:03Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+        // For instance, we return true for both of these:
+        //
+        //    partial partial<T>()
+        //    partial partail<T> partial()


partail [](start = 22, length = 7)

typo?

jcouv · 2019-09-12T17:52:51Z

The parsing of modifiers has been updated in the meantime, causing some conflicts.

alrz · 2019-09-29T14:38:01Z

some of test failures are there just because the default langversion doesn't include preview features. should we adjust those tests or include preview in default?

jcouv · 2019-09-30T17:34:47Z

@alrz The tests should be adjusted to use Preview language version. Once we lock the release each feature goes into, those tests will be updated with a specific language version (TestOptions.Regular9 and such).

alrz · 2019-12-18T17:06:43Z

We probably need to consider the data modifier here as well since it's a contextual keyword.

/cc @agocke

agocke · 2019-12-18T22:13:00Z

@alrz Agreed. I'm going to regard this feature as necessary if we want to add a data keyword. I'm not going to try to design a required ordering for all these keywords.

agocke · 2020-02-28T23:15:38Z

@alrz If you don't mind I'm gonna rebase this to master in a new PR. I'm gonna build on it for my records work.

alrz · 2020-02-29T05:40:16Z

Sure. I think we can close this one now. Thanks.

I'll keep the branch for you to rebase.

agocke · 2020-03-02T19:03:26Z

Thanks! Draft PR #42097

I'll keep your code and just add any changes needed for records.

alrz force-pushed the mod-ordering branch from 132ccbb to df79df5 Compare December 2, 2017 22:48

alrz changed the base branch from master to features/compiler December 3, 2017 01:47

alrz force-pushed the mod-ordering branch from 6d4dc7d to 4db063d Compare December 3, 2017 01:47

alrz requested a review from a team as a code owner December 3, 2017 01:47

alrz force-pushed the mod-ordering branch from 4db063d to 9eb2e9e Compare December 4, 2017 10:09

jasonmalinowski requested review from a team and removed request for a team December 4, 2017 22:35

gafter added the Area-Compilers label Dec 5, 2017

gafter assigned jaredpar Dec 5, 2017

agocke requested changes Dec 8, 2017

View reviewed changes

jcouv modified the milestones: 15.6, 15.7 Dec 8, 2017

alrz force-pushed the mod-ordering branch from e4add68 to cea0ae7 Compare December 9, 2017 03:21

jcouv added this to the 16.4 milestone Sep 6, 2019

jcouv reviewed Sep 12, 2019

View reviewed changes

alrz added 10 commits September 29, 2019 15:03

Relax 'ref' and 'partial' modifier ordering on type declarations

6585f98

Update resources

997fc81

Fixup DIM tests

05874cc

Add test for partial methods

02ff460

Cleanup ref struct parsing

2152d84

Sort out feature flag checks

608ac79

Fix bug on additional modifier detection

c28dacf

Add an additional test.

c86da15

Update resources

44a0fb8

Change required language version to preview

008e19d

alrz force-pushed the mod-ordering branch from df47c79 to 008e19d Compare September 29, 2019 11:59

alrz added 2 commits September 29, 2019 15:51

Fixup tests

92b2f32

Fix typo

9d35c08

alrz force-pushed the mod-ordering branch from ac7626b to 9d35c08 Compare September 29, 2019 12:31

Adjust failing tests to use preview language version

c56b205

alrz requested review from jcouv and removed request for a team October 3, 2019 10:07

jinujoseph removed this from the 16.4 milestone Dec 11, 2019

alrz closed this Feb 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax ordering constraints around 'ref' and 'partial' modifiers #23533

Relax ordering constraints around 'ref' and 'partial' modifiers #23533

alrz commented Dec 2, 2017 •

edited by jcouv

Loading

alrz commented Dec 4, 2017 •

edited by jcouv

Loading

jasonmalinowski commented Dec 4, 2017 •

edited by jcouv

Loading

alrz commented Dec 4, 2017 •

edited by jcouv

Loading

jasonmalinowski commented Dec 4, 2017 •

edited by jcouv

Loading

alrz commented Dec 5, 2017 •

edited by jcouv

Loading

gafter commented Dec 5, 2017 •

edited by jcouv

Loading

alrz commented Dec 7, 2017 •

edited by jcouv

Loading

agocke Dec 8, 2017

agocke Dec 8, 2017

agocke Dec 8, 2017 •

edited by jcouv

Loading

alrz Dec 8, 2017 •

edited

Loading

agocke Dec 8, 2017

alrz commented Dec 9, 2017 •

edited by jcouv

Loading

sharwell commented Dec 11, 2017 •

edited by jcouv

Loading

tmeschter commented Dec 11, 2017 •

edited by jcouv

Loading

AlekseyTs commented Dec 11, 2017 •

edited by jcouv

Loading

jcouv commented Dec 11, 2017 •

edited

Loading

alrz commented Dec 14, 2017 •

edited by jcouv

Loading

tmeschter commented Dec 14, 2017 •

edited by jcouv

Loading

tmeschter commented Dec 14, 2017 •

edited by jcouv

Loading

nguerrera commented Dec 14, 2017 •

edited by jcouv

Loading

jcouv Sep 12, 2019 •

edited

Loading

jcouv commented Sep 12, 2019

alrz commented Sep 29, 2019

jcouv commented Sep 30, 2019

alrz commented Dec 18, 2019

agocke commented Dec 18, 2019

agocke commented Feb 28, 2020

alrz commented Feb 29, 2020 •

edited

Loading

agocke commented Mar 2, 2020

Relax ordering constraints around 'ref' and 'partial' modifiers #23533

Relax ordering constraints around 'ref' and 'partial' modifiers #23533

Conversation

alrz commented Dec 2, 2017 • edited by jcouv Loading

alrz commented Dec 4, 2017 • edited by jcouv Loading

jasonmalinowski commented Dec 4, 2017 • edited by jcouv Loading

alrz commented Dec 4, 2017 • edited by jcouv Loading

jasonmalinowski commented Dec 4, 2017 • edited by jcouv Loading

alrz commented Dec 5, 2017 • edited by jcouv Loading

gafter commented Dec 5, 2017 • edited by jcouv Loading

alrz commented Dec 7, 2017 • edited by jcouv Loading

agocke Dec 8, 2017

Choose a reason for hiding this comment

agocke Dec 8, 2017

Choose a reason for hiding this comment

agocke Dec 8, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

alrz Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

agocke Dec 8, 2017

Choose a reason for hiding this comment

alrz commented Dec 9, 2017 • edited by jcouv Loading

sharwell commented Dec 11, 2017 • edited by jcouv Loading

tmeschter commented Dec 11, 2017 • edited by jcouv Loading

AlekseyTs commented Dec 11, 2017 • edited by jcouv Loading

jcouv commented Dec 11, 2017 • edited Loading

alrz commented Dec 14, 2017 • edited by jcouv Loading

tmeschter commented Dec 14, 2017 • edited by jcouv Loading

tmeschter commented Dec 14, 2017 • edited by jcouv Loading

nguerrera commented Dec 14, 2017 • edited by jcouv Loading

jcouv Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv commented Sep 12, 2019

alrz commented Sep 29, 2019

jcouv commented Sep 30, 2019

alrz commented Dec 18, 2019

agocke commented Dec 18, 2019

agocke commented Feb 28, 2020

alrz commented Feb 29, 2020 • edited Loading

agocke commented Mar 2, 2020

alrz commented Dec 2, 2017 •

edited by jcouv

Loading

alrz commented Dec 4, 2017 •

edited by jcouv

Loading

jasonmalinowski commented Dec 4, 2017 •

edited by jcouv

Loading

alrz commented Dec 4, 2017 •

edited by jcouv

Loading

jasonmalinowski commented Dec 4, 2017 •

edited by jcouv

Loading

alrz commented Dec 5, 2017 •

edited by jcouv

Loading

gafter commented Dec 5, 2017 •

edited by jcouv

Loading

alrz commented Dec 7, 2017 •

edited by jcouv

Loading

agocke Dec 8, 2017 •

edited by jcouv

Loading

alrz Dec 8, 2017 •

edited

Loading

alrz commented Dec 9, 2017 •

edited by jcouv

Loading

sharwell commented Dec 11, 2017 •

edited by jcouv

Loading

tmeschter commented Dec 11, 2017 •

edited by jcouv

Loading

AlekseyTs commented Dec 11, 2017 •

edited by jcouv

Loading

jcouv commented Dec 11, 2017 •

edited

Loading

alrz commented Dec 14, 2017 •

edited by jcouv

Loading

tmeschter commented Dec 14, 2017 •

edited by jcouv

Loading

tmeschter commented Dec 14, 2017 •

edited by jcouv

Loading

nguerrera commented Dec 14, 2017 •

edited by jcouv

Loading

jcouv Sep 12, 2019 •

edited

Loading

alrz commented Feb 29, 2020 •

edited

Loading