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

readonly ref local functions, error tolerance in parsing, more tests. #17223

Merged
merged 2 commits into from
Feb 21, 2017

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Feb 17, 2017

Correclty handle local functions.

Correclty handle local functions.
@VSadov
Copy link
Member Author

VSadov commented Feb 17, 2017

@OmarTawfik

@VSadov
Copy link
Member Author

VSadov commented Feb 17, 2017

@dotnet/roslyn-compiler - FYI

Assert.NotNull(p.GetMethod);
Assert.False(p.GetMethod.IsImplicitlyDeclared);
Assert.True(p.IsExpressionBodied);
Assert.Equal(RefKind.Ref, p.GetMethod.RefKind);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 21, 2017

Choose a reason for hiding this comment

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

Assert.Equal(RefKind.Ref, p.GetMethod.RefKind); [](start = 12, length = 47)

Is this correct? Should this be a "readonly ref"? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

This is correct for now. Binding is NYI and falls back on regular ref returns.
The test will conveniently fail when binding is implemented.


In reply to: 102295224 [](ancestors = 102295224)

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we should probably have a "Prototype" comment here (similar to ReadonlyRefReturningDelegate test added in this PR), so that we could ensure the test is adjusted at the end.


In reply to: 102322176 [](ancestors = 102322176,102295224)

{
Error(diagnostics, ErrorCode.ERR_BadAsyncLocalType, declarator);
var refKeyword = typeSyntax.GetFirstToken();
diagnostics.Add(ErrorCode.ERR_UnexpectedToken, refKeyword.GetLocation(), refKeyword.ToString());
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 21, 2017

Choose a reason for hiding this comment

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

diagnostics.Add(ErrorCode.ERR_UnexpectedToken, refKeyword.GetLocation(), refKeyword.ToString()); [](start = 16, length = 96)

Could you please point to the test that goes through this code path? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

ReadonlyRefReturn_UnexpectedBindTime

We allow readonly ref locals at parse time for error recovery purposes, but will fail at bind time.

This is the same strategy as used for regular ref. We parse it as RefType in more places than where it is semantically allowed, but fail to bind with this error.

Also there is a possibility we would allow readonly ref locals. That is another reason for having the check at bind time.


In reply to: 102299405 [](ancestors = 102299405)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am probably missing something, but I don't see this error reported for a local declaration in that test.


In reply to: 102323203 [](ancestors = 102323203,102299405)

Copy link
Member Author

Choose a reason for hiding this comment

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

Because binding is NYI and we fallback on "Ref", which is valid.
I will add PROTOTYPE note


In reply to: 102336648 [](ancestors = 102336648,102323203,102299405)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Feb 21, 2017

Consider adding tests for deconstruction declarations, out var declarations and declaration patterns. #Closed

@VSadov
Copy link
Member Author

VSadov commented Feb 21, 2017

I also have some more test ideas
Entered #17285 to follow up on that.


In reply to: 281458663 [](ancestors = 281458663)

@VSadov VSadov changed the title More parsing tests for readonly ref returns readonly ref local functions, error tolerance in parsing, more tests. Feb 21, 2017
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM

@VSadov VSadov merged commit 819e096 into dotnet:features/readonly-ref Feb 21, 2017
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.

None yet

3 participants