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: ${(…; …)} & multiline expr handling #16666

Merged
merged 12 commits into from Feb 8, 2024

Conversation

brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Feb 7, 2024

Another followup to #16079.

Description

  • Keep parens around sequential exprs in interpolated strings.

    Source

    $"{((); ())}"

    Before

    $"{(); ()}"
    // error FS0010: Unexpected symbol ';' in interaction. Expected interpolated string (final part), interpolated string (part) or other token.

    After

    $"{((); ())}"
  • Handle more multiline parenthesization scenarios.

    The snipping tool is not working for me right now for some reason, but here is an example of the kind of multiline parenthesized expression that is now handled correctly:

    Source

    let _ =
      let x = 3
      (
          let y = 99
          y - x
      )

    Before

    let _ =
      let x = 3
      
          let y = 99
          y - x
      
    // error FS0010: Unexpected keyword 'let' or 'use' in binding. Expected incomplete structured construct at or before this point or other token.

    After

    let _ =
      let x = 3
      let y = 99
      y - x

    I also introduced some new logic to handle scenarios like the following, where removing the parentheses would result in outer bindings being shadowed, e.g.:

    let _ =
      let x = 1
      let y = 2
      (
          let y = -2
          printfn $"{y}"
      )
      x + y // y is 2 here; it would become -2 if we removed the parens, so we must keep them.
  • Trim extra whitespace between the parens and the inner constructs, e.g.:

    let _ = (   3 )

    let _ = 3

Checklist

  • Test cases added.
  • Release notes entry updated.

Notes

@nojaf I moved the innards of the ParsedInput module from #16462 into a SyntaxNodes module, exposed only internally for now. The ParsedInput module now simply delegates to that.

bba8e2a shows an example of the "composability" that I was trying to gesture at before.

Copy link
Contributor

github-actions bot commented Feb 7, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.300.md
vsintegration/src docs/release-notes/.VisualStudio/17.10.md

@brianrourkeboll brianrourkeboll marked this pull request as ready for review February 7, 2024 17:04
@brianrourkeboll brianrourkeboll requested a review from a team as a code owner February 7, 2024 17:04
Copy link
Member

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

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

For an internal module, this is exceptionally well detailed API description, thank you

abonie
abonie previously approved these changes Feb 8, 2024
@abonie abonie self-requested a review February 8, 2024 12:13
@abonie abonie dismissed their stale review February 8, 2024 12:14

Does this issue actually repro?

@abonie
Copy link
Member

abonie commented Feb 8, 2024

Hey, how do I repro this? Tried the code and it does not cause any issues?

@brianrourkeboll
Copy link
Contributor Author

brianrourkeboll commented Feb 8, 2024

@abonie

Hey, how do I repro this? Tried the code and it does not cause any issues?

With an extension built from the current main (and probably also in the latest VS preview), if you have the unnecessary parentheses code fix enabled, it should offer to remove the parens in these examples:

$"{((); ())}"
let _ =
  let x = 3
  (
      let y = 99
      y - x
  )

but the resulting code will be invalid.

image

image

@brianrourkeboll
Copy link
Contributor Author

@T-Gro

For an internal module, this is exceptionally well detailed API description, thank you

It's just a copy of the docs for the public ParsedInput module, which is now essentially just a wrapper for the SyntaxNodes one :)

@abonie
Copy link
Member

abonie commented Feb 8, 2024

My bad, didn't have it enabled, thanks for explaining :)

@psfinaki psfinaki enabled auto-merge (squash) February 8, 2024 13:49
@psfinaki
Copy link
Member

psfinaki commented Feb 8, 2024

Great stuff, as usual :)

@psfinaki psfinaki merged commit b4e5fba into dotnet:main Feb 8, 2024
28 checks passed
@brianrourkeboll brianrourkeboll deleted the parens-a-couple-more branch February 8, 2024 17:16
brianrourkeboll added a commit to brianrourkeboll/fsharp that referenced this pull request Feb 29, 2024
* The logic in dotnet#16666 addressed some multiline parentheses removal
  issues, but it was slightly too aggressive and sometimes shifted
  lines farther than was necessary or safe.
psfinaki pushed a commit that referenced this pull request Mar 4, 2024
* Shift paren contents less aggressively

* The logic in #16666 addressed some multiline parentheses removal
  issues, but it was slightly too aggressive and sometimes shifted
  lines farther than was necessary or safe.

* Update release notes

---------

Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
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.

None yet

4 participants