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

Add parser recovery for unfinished interface implementation #10416

Merged
merged 3 commits into from
Nov 12, 2020

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Nov 9, 2020

Adds recovery for unfinished interface ... with that contains no members but is already given with keyword.

Example 1: before (interface impl is not parsed at all, subsequent declarations are corrupted):
Screenshot 2020-11-09 at 22 39 23

Example 1: after (everything is parsed, type check results are available for the next declaration):
Screenshot 2020-11-09 at 22 42 36

The main problem with the example above is when with is present without any members, it simply fails to parse, skips other declarations, and no error about missing members is reported at all (for a user or tooling to fix it).

Example 2: before (subsequent member is considered to be interface part):
Screenshot 2020-11-09 at 22 40 30

Example 2: after (subsequent member belongs to the type declaration):
Screenshot 2020-11-09 at 22 40 45

The current behaviour in the second example may actually be an intended behaviour. I've initially thought that from my experience it usually did more harm than good, since subsequence members do have the correct offset for the type declaration, but now I'm thinking it shouldn't have been changed. This part can easily be reverted by using < in the LexFilter change instead. @dsyme @cartermp What do you think about this change?

@auduchinok auduchinok force-pushed the interfaceImpl-withRecovery branch from 86913cd to dd887b2 Compare November 9, 2020 20:28
@cartermp
Copy link
Contributor

cartermp commented Nov 9, 2020

but now I'm thinking it shouldn't have been changed.

Yeah, this one is tricky. I think I would prefer the old behavior because out of the two following scenarios:

  1. User already has a member M scoped to the class, then adds the interface declaration above the member P
  2. User adds the interface declaration, then adds a new member M to implement that interface but doesn't have their indentation correct (e.g., has smart indentation turned off in VS)

The second one feels more likely to happen. But neither is really "correct".

@auduchinok
Copy link
Member Author

If returning the old behaviour, subsequent members have to be member declarations that are allowed in interface implementations, which limits recovery severely:
Screenshot 2020-11-10 at 13 08 33

I'm thinking about other approaches.

@auduchinok
Copy link
Member Author

auduchinok commented Nov 10, 2020

OK, subsequent members are parsed properly now and belong to the type, and the indentation warning in this particular case is reported by the parser instead of LexFilter.

Screenshot 2020-11-10 at 15 41 54

Screenshot 2020-11-10 at 15 45 43

The warning range is larger with this approach, but a similar range is used in some other places in the compiler already and is probably not that bad. We don't have the first token range info at this point anymore anyway.

Technically this is a breaking change, since it was possible to implement interfaces without further indenting members, but I much prefer how it's handled now with an explicit error and properly parsing all type members around.

@auduchinok
Copy link
Member Author

auduchinok commented Nov 10, 2020

@cartermp @vzarytovskii The test failures seem unrelated, have you seen this before?

@vzarytovskii
Copy link
Member

@cartermp @vzarytovskii The test failures seem unrelated, have you seen this before?

That is a weird one, looks like some agent hiccup, I'll take a look.

@auduchinok auduchinok closed this Nov 10, 2020
@auduchinok auduchinok reopened this Nov 10, 2020
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Thanks, this is approved pending tests passing. I like the adjustment!

@auduchinok auduchinok force-pushed the interfaceImpl-withRecovery branch from 39ef8c0 to 7a5ca77 Compare November 12, 2020 08:37
@auduchinok
Copy link
Member Author

This is ready.

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

Looks good to me, awesome

@TIHan TIHan merged commit d7d91d9 into dotnet:main Nov 12, 2020
@auduchinok auduchinok deleted the interfaceImpl-withRecovery branch November 12, 2020 16:37
@cartermp cartermp added this to the 16.9 milestone Nov 17, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
…0416)

* Add parser recovery for unfinished interface implementation

* Report indentation problem via parser

* Update public area
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants