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

Parser: parse primary ctor params as normal patterns #16425

Merged
merged 17 commits into from
Jan 10, 2024

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Dec 12, 2023

Prior to this PR the compiler had two way of parsing patterns:

  • normal patterns (used in match expressions, let bindings, member declarations, and so on)
  • 'simple' patterns (used in primary constructors)

The idea of the 'simple' patterns was to limit what patterns are allowed in the primary constructors. These are parsed by special parser rules that allow parsing a limited subset of the patterns. Using separate rules poses several issues:

  • some of the pattern parsing improvements are not used in primary constructors parsing
  • when a complex pattern is written in a constructor, parser fails miserably instead of reporting a good error

Simple patterns are also used internally during checking of various match/let/function/fun expressions in some cases, and the parsed patterns are converted into 'simple' ones along the way.

This PR removes the special pattern parsing in primary constructors:

  • patterns are parsed as normal patterns
  • the parsed patterns are converted to 'simple' ones, like it's done for other cases
  • errors are reported for compiler generated patterns (i.e. the ones that aren't considered simple and need wrapping into as _arg1 or a similar pattern during conversion)

This fixes parsing/analysis for cases like this:

type T(a,) = class end
type T(a, , c) = class end
type T(a, (b, c)) = class end

It also fixes issues that prevented further analysis of a type declaration:

Screenshot 2023-12-12 at 16 31 42

@auduchinok auduchinok requested a review from a team as a code owner December 12, 2023 15:32
@auduchinok
Copy link
Member Author

This is ready. 🎉

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Removing parser code? I don't believe you :D Haven't ever seen this happening.

src/Compiler/pars.fsy Show resolved Hide resolved
Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Thanks, looks awesome!

@auduchinok
Copy link
Member Author

auduchinok commented Dec 27, 2023

This is ready again (after resolving the upstream conflicts) 🙂

Copy link
Contributor

github-actions bot commented Jan 3, 2024

✅ No release notes required

Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Great work!

@T-Gro T-Gro enabled auto-merge (squash) January 10, 2024 08:40
@T-Gro T-Gro merged commit e249689 into dotnet:main Jan 10, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants