Skip to content

Conversation

@auduchinok
Copy link
Member

@auduchinok auduchinok commented Jun 7, 2023

This is the first part of the changes needed for fsharp/fslang-suggestions#1273.

I'm going to split this work into several PRs, so it's easier to deal with new recovery rules separately (because some of the changes require updating various parts of the tooling support), and because I want to start testing it early.

The most common case where indentation warnings are raised is adding a new sequence context, and it was possible to change it in an unobtrusive way, only passing an additional token for the recovery. With this change a new recovery context is pushed to the stack, and then is immediately popped out by the unindented token, making LexFilter produce all the correct begin and end-like tokens.

The parser remains compatible with the current language rules, and new language rules are enabled with a switch.
The recently added recovery rules got picked up automatically, so we got significantly better recovery for things like incomplete binary expressions without doing extra work on top of existing changes.

The next steps for the following PRs are going to be these:

Below are some demos illustrating the changes:

Demo 1 (subsequent code is wrongly parsed as a single expression)

Before:
indentationBefore02

After:
indentationAfter02

Demo 2 (no recovery of the subsequent code, often wrong analysis/lenses)

Before:
indentationBefore03

After:
indentationAfter03

There're some open questions:

  • Do we want to have the parser tests for both the old and the new rules? We've only checked preview version before here, and it seems practical. We can duplicate some of the tests with the old rules enabled, but I'd like to get some help with the testing framework if we decide to do it.

@auduchinok auduchinok requested a review from a team as a code owner June 7, 2023 11:24
@kerams
Copy link
Contributor

kerams commented Jun 7, 2023

Am I correct in assuming that #7078 is also not going to be an issue to the same degree anymore?

@auduchinok
Copy link
Member Author

auduchinok commented Jun 7, 2023

Am I correct in assuming that #7078 is also not going to be an issue to the same degree anymore?

Yes, it's exactly the kind of the issues that this language change is trying to fix.

@auduchinok auduchinok force-pushed the lexFilter-strict3 branch from 3fc9281 to 6b363c9 Compare June 7, 2023 13:34
@auduchinok
Copy link
Member Author

auduchinok commented Jun 7, 2023

Am I correct in assuming that #7078 is also not going to be an issue to the same degree anymore?

@kerams I've just checked the repro from #7078 and it works perfectly with this change:

Demo typing

indentationAfterTyping01

Update: I've just noticed that the repro from #7078 uses incorrect indent, i.e. let is typed inside a union type, which is not valid code. In my experiment I've tried it on the module scope indentation instead. Nevertheless, before this change even when typing at the module scope, everything get unfolded and broken during typing.

Here are screenshots during the typing before the change:

Screenshot 2023-06-07 at 18 20 33 Screenshot 2023-06-07 at 18 20 40

This change is mostly aimed at fixing the parser for unfinished code, but recovery for incorrect code like let inside unions can also be added to the parser separately, but it would require this change anyway.

@auduchinok auduchinok closed this Jun 7, 2023
@auduchinok auduchinok reopened this Jun 7, 2023
@auduchinok
Copy link
Member Author

It's ready.

@edgarfgp
Copy link
Contributor

edgarfgp commented Jun 8, 2023

This is fantastic work. Thanks 🙏 . Will these changes will improve the case of an unfinished code like :
Screenshot 2023-06-08 at 11 32 24

@auduchinok
Copy link
Member Author

auduchinok commented Jun 8, 2023

Will these changes will improve the case of an unfinished code like :

@edgarfgp It may help with significant part of problems in examples like this, but additional work is still needed in both the parser and the type checker. What's important is it makes many cases possible to recover that weren't previously, because of the underlying language rules change, which in a way gives us a better foundation to improve the parser on.

@auduchinok auduchinok force-pushed the lexFilter-strict3 branch from adbe8be to e941852 Compare June 8, 2023 15:52
@auduchinok
Copy link
Member Author

auduchinok commented Jun 13, 2023

I've started working on the changes needed for fsharp/fslang-suggestions#1273 (comment), and there're some conflicts due to the changes in the rules and recovery.

May I ask for #15369 and this PR to be reviewed and merged, so it's easier to do the subsequent work, please? 🙂

@auduchinok
Copy link
Member Author

auduchinok commented Jun 14, 2023

Yay, it's green again! 🎉

I've reverted a part from #15369 to fix the code completion tests. Both this and that PR do correct things, but the code completion logic accidentally relies on a particular tree shape (unfinished node ranges, to be specific) and that shouldn't really be changed as part of this PR. I'll try to either reapply the removed part from #15369 or will just store the explicit done range separately later, when the parser/lexfilter changes are more complete.

@abonie abonie merged commit d1ca485 into dotnet:main Jun 14, 2023
@auduchinok auduchinok deleted the lexFilter-strict3 branch June 14, 2023 13:27
@DedSec256
Copy link
Contributor

Great work 👍

@edgarfgp
Copy link
Contributor

I can not wait to use this changes 🥳 . It is going to make it easier to work and contribute to the compiler itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

8 participants