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.

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

Move IDS_FeatureRefLocalsReturns out of the parser #65710

Merged
merged 140 commits into from
Dec 3, 2022

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Dec 1, 2022

This is the final parser change that i'm aware of where we produce different trees based on language version. W00t!

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs Outdated Show resolved Hide resolved
_refKind = RefKind.None;
}

syntax.ReturnType.SkipRef(_declarationDiagnostics, out _refKind);
Copy link
Contributor

@RikkiGibson RikkiGibson Dec 2, 2022

Choose a reason for hiding this comment

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

would _refKind = syntax.ReturnType.GetRefKind(DiagnosticBag) be more appropriate here?

Copy link
Member Author

Choose a reason for hiding this comment

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

woudln't that avoid the error about using a ref-return on a local function?

@@ -761,7 +761,7 @@ private string GetDebuggerDisplay()
bool isVar;
bool isConst = false;
AliasSymbol alias;
var declType = BindVariableTypeWithAnnotations(component.Designation, diagnostics, component.Type.SkipScoped(out _).SkipRef(out _), ref isConst, out isVar, out alias);
var declType = BindVariableTypeWithAnnotations(component.Designation, diagnostics, component.Type.SkipScoped(out _).SkipRef(), ref isConst, out isVar, out alias);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seemed like in some places, we are skipping ref, but some other check is deciding conditionally whether ref is OK in that position. For example, in BindVariableTypeWithAnnotations. It looks like some of the time you deliberately called the overload to pass diagnostics: null, like in SourceLocalSymbol.

It looks like the SkipRef overload that takes a diagnostic bag is specifically checking for the ref locals and returns feature. I am wondering if the name of this method should be changed, e.g. SkipRef() with no parameters sticks around, and SkipRef(DiagnosticBag, out RefKind gets renamed to something like SkipRefInLocalOrReturn(DiagnosticBag, out RefKind). Then it might be slightly easier to tell at a glance why it's expected to pass a diagnostic bag or not.

@CyrusNajmabadi
Copy link
Member Author

@RikkiGibson thanks. this got a lot nicer!

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) December 3, 2022 00:08
@CyrusNajmabadi CyrusNajmabadi merged commit eebc8b7 into dotnet:main Dec 3, 2022
@ghost ghost added this to the Next milestone Dec 3, 2022
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM

@CyrusNajmabadi CyrusNajmabadi deleted the langVersionChecksCont4 branch December 3, 2022 01:42
@Cosifne Cosifne modified the milestones: Next, 17.5 P3 Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants