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

Parens: more comprehensive pattern tests #16313

Merged
merged 15 commits into from
Nov 24, 2023

Conversation

brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Nov 20, 2023

Another followup to #16079.

  • Replace most parenthesized pattern tests with a more comprehensive and organized set of generated tests.
  • Speed up all the paren tests by 2–3× (see d1773e7).
  • Fix a few stray bugs caught by the new tests.

Let me know if you think this is overkill. It probably doesn't make sense to do the exact same thing for expressions (I would do that in a separate PR), since the number of tests would probably be massive, but I have half a mind to try something similar using FsCheck instead, even if it is not run on every commit.

The tests in this PR are grouped as follows:

  • Singly nested patterns tests atomic or nullary patterns nested one level deep inside of all non-nullary patterns (patterns in which other patterns can be nested).
  • Deeply nested patterns tests all non-atomic, non-nullary patterns nested inside of all non-nullary patterns.
  • Miscellaneous patterns tests some of the compiler oddities I found.
  • Each of the other groupings tests top-level atomic and non-atomic patterns in noteworthy non-SynPat contexts, among which there are slight differences in when parens are required: SynBinding, SynMatchClause, SynExpr.LetOrUse, SynExpr.LetOrUseBang, SynExpr.Lambda.

image

Examples

image

image

@brianrourkeboll brianrourkeboll marked this pull request as ready for review November 21, 2023 13:29
@brianrourkeboll brianrourkeboll requested a review from a team as a code owner November 21, 2023 13:29
@brianrourkeboll brianrourkeboll changed the title Parens: cleaned up/more comprehensive pattern tests Parens: more comprehensive pattern tests Nov 21, 2023
@psfinaki
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Member

psfinaki commented Nov 21, 2023

Thanks for extra tests! Well as long as the tests maintain structure, don't run too long and are not flaky, I am fine with that. I wonder if they can be somehow reorged like "common scenarios" vs "edge-case scenarios"?

* This optimization could in theory be used for all tests in this
  project if we set up some kind of solution cache keyed by project &
  editor settings, but it looks like it would require some refactoring
  in RoslynTestHelpers.
@brianrourkeboll
Copy link
Contributor Author

brianrourkeboll commented Nov 22, 2023

@psfinaki I added some comments to the test code for clarity and updated the PR description. Let me know if you think there's anything else I should do to make things clearer.

…Although I guess there are still some more places I could add comments—e.g., why a dangling typed expression is not allowed in a match clause (because the match clause's -> would be parsed as part of the type), etc.

@psfinaki
Copy link
Member

@brianrourkeboll I think this is good enough for now. We can always reassess the test structure in followups :)

@T-Gro T-Gro merged commit 2834b34 into dotnet:main Nov 24, 2023
25 checks passed
@brianrourkeboll brianrourkeboll deleted the parens-more-cleanup branch November 24, 2023 16:05
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.

3 participants