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 range of do keyword to SynExpr.Do. #10853

Closed
wants to merge 1 commit into from

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Jan 8, 2021

This is related to #10198.
A different keyword than mentioned in the issue but the concept still stands.

Having this information would be useful to solve fsprojects/fantomas#1343.

Let me know what you think @cartermp.

@nojaf nojaf force-pushed the do-keyword-range branch from dcde857 to b01a812 Compare January 8, 2021 14:54
@@ -695,6 +695,7 @@ type SynExpr =
/// F# syntax: do expr
| Do of
expr: SynExpr *
Copy link
Member

Choose a reason for hiding this comment

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

this makes me wish that we could include parameter-level documentation on DU members specifically and have them rolled-up somehow.

@baronfel
Copy link
Member

baronfel commented Jan 8, 2021

This looks sane to me. I'm loving how your experiences in Fantomas flow back here in terms of enhancements and improvements to the AST structures!

@auduchinok
Copy link
Member

auduchinok commented Jan 13, 2021

I've assumed do is always at the start of SynExpr.Do when it's DoBinding, so it's trivial to calculate its range. Is there a case where it's not true?

I've previously proposed this fix: #10365, so it'd be possible to calculate do from expression range, so I don't think storing additional field is needed.

@nojaf
Copy link
Contributor Author

nojaf commented Jan 13, 2021

@auduchinok, please consider the following example:

let a =
    // Import new etags of pushed items
    do
        pushed
        |> Option.bind Dto.changesAsImport
        |> Option.iter (fun changes -> update (Import changes) |> ignore)

matching AST:

ImplFile
  (ParsedImplFileInput
     ("tmp.fsx", true, QualifiedNameOfFile Tmp$fsx, [], [],
      [SynModuleOrNamespace
         ([Tmp], false, AnonModule,
          [Let
             (false,
              [Binding
                 (None, NormalBinding, false, false, [],
                  PreXmlDoc ((1,4), FSharp.Compiler.XmlDoc+XmlDocCollector),
                  SynValData
                    (None, SynValInfo ([], SynArgInfo ([], false, None)), None),
                  Named
                    (Wild tmp.fsx (1,4--1,5) IsSynthetic=false, a, false, None,
                     tmp.fsx (1,4--1,5) IsSynthetic=false), None,
                  Do
                    (App
                       (NonAtomic, false,
                        App
                          (NonAtomic, true, Ident op_PipeRight,
                           App
                             (NonAtomic, false,
                              App
                                (NonAtomic, true, Ident op_PipeRight,
                                 Ident pushed,
                                 tmp.fsx (4,8--5,10) IsSynthetic=false),
                              App
                                (NonAtomic, false,
                                 LongIdent
                                   (false,
                                    LongIdentWithDots
                                      ([Option; bind],
                                       [tmp.fsx (5,17--5,18) IsSynthetic=false]),
                                    None, tmp.fsx (5,11--5,22) IsSynthetic=false),
                                 LongIdent
                                   (false,
                                    LongIdentWithDots
                                      ([Dto; changesAsImport],
                                       [tmp.fsx (5,26--5,27) IsSynthetic=false]),
                                    None, tmp.fsx (5,23--5,42) IsSynthetic=false),
                                 tmp.fsx (5,11--5,42) IsSynthetic=false),
                              tmp.fsx (4,8--5,42) IsSynthetic=false),
                           tmp.fsx (4,8--6,10) IsSynthetic=false),
                        App
                          (NonAtomic, false,
                           LongIdent
                             (false,
                              LongIdentWithDots
                                ([Option; iter],
                                 [tmp.fsx (6,17--6,18) IsSynthetic=false]), None,
                              tmp.fsx (6,11--6,22) IsSynthetic=false),
                           Paren
                             (Lambda
                                (false, false,
                                 SimplePats
                                   ([Id
                                       (changes, None, false, false, false,
                                        tmp.fsx (6,28--6,35) IsSynthetic=false)],
                                    tmp.fsx (6,28--6,35) IsSynthetic=false),
                                 App
                                   (NonAtomic, false,
                                    App
                                      (NonAtomic, true, Ident op_PipeRight,
                                       App
                                         (NonAtomic, false, Ident update,
                                          Paren
                                            (App
                                               (NonAtomic, false, Ident Import,
                                                Ident changes,
                                                tmp.fsx (6,47--6,61) IsSynthetic=false),
                                             tmp.fsx (6,46--6,47) IsSynthetic=false,
                                             Some
                                               tmp.fsx (6,61--6,62) IsSynthetic=false,
                                             tmp.fsx (6,46--6,62) IsSynthetic=false),
                                          tmp.fsx (6,39--6,62) IsSynthetic=false),
                                       tmp.fsx (6,39--6,65) IsSynthetic=false),
                                    Ident ignore,
                                    tmp.fsx (6,39--6,72) IsSynthetic=false),
                                 Some
                                   ([Named
                                       (Wild
                                          tmp.fsx (6,28--6,35) IsSynthetic=false,
                                        changes, false, None,
                                        tmp.fsx (6,28--6,35) IsSynthetic=false)],
                                    App
                                      (NonAtomic, false,
                                       App
                                         (NonAtomic, true, Ident op_PipeRight,
                                          App
                                            (NonAtomic, false, Ident update,
                                             Paren
                                               (App
                                                  (NonAtomic, false,
                                                   Ident Import, Ident changes,
                                                   tmp.fsx (6,47--6,61) IsSynthetic=false),
                                                tmp.fsx (6,46--6,47) IsSynthetic=false,
                                                Some
                                                  tmp.fsx (6,61--6,62) IsSynthetic=false,
                                                tmp.fsx (6,46--6,62) IsSynthetic=false),
                                             tmp.fsx (6,39--6,62) IsSynthetic=false),
                                          tmp.fsx (6,39--6,65) IsSynthetic=false),
                                       Ident ignore,
                                       tmp.fsx (6,39--6,72) IsSynthetic=false)),
                                 tmp.fsx (6,24--6,72) IsSynthetic=false),
                              tmp.fsx (6,23--6,24) IsSynthetic=false,
                              Some tmp.fsx (6,72--6,73) IsSynthetic=false,
                              tmp.fsx (6,23--6,73) IsSynthetic=false),
                           tmp.fsx (6,11--6,73) IsSynthetic=false),
                        tmp.fsx (4,8--6,73) IsSynthetic=false),
                     tmp.fsx (4,8--6,73) IsSynthetic=false),
                  tmp.fsx (1,4--1,5) IsSynthetic=false,
                  DebugPointAtBinding tmp.fsx (1,0--6,73) IsSynthetic=false)],
              tmp.fsx (1,0--6,73) IsSynthetic=false)], PreXmlDocEmpty, [], None,
          tmp.fsx (1,0--7,0) IsSynthetic=false)], (true, true)))

The range of SynExpr.Do is 4,8--6,73.
My keyword is on line 3.

@auduchinok
Copy link
Member

@nojaf Thanks for the example. If the do keyword ends up outside the range it sounds simply like a wrong range is reported for the expression. I think the correct thing to do would be fixing the range instead.

@nojaf
Copy link
Contributor Author

nojaf commented Jan 14, 2021

Hi @auduchinok.
You might be on to something here.
Take the following example:

let a = 
    do
        foobar
    do!
        foobarBang

AST

ImplFile
  (ParsedImplFileInput
     ("tmp.fsx", true, QualifiedNameOfFile Tmp$fsx, [], [],
      [SynModuleOrNamespace
         ([Tmp], false, AnonModule,
          [Let
             (false,
              [Binding
                 (None, NormalBinding, false, false, [],
                  PreXmlDoc ((1,4), FSharp.Compiler.XmlDoc+XmlDocCollector),
                  SynValData
                    (None, SynValInfo ([], SynArgInfo ([], false, None)), None),
                  Named
                    (Wild tmp.fsx (1,4--1,5) IsSynthetic=false, a, false, None,
                     tmp.fsx (1,4--1,5) IsSynthetic=false), None,
                  Sequential
                    (Both, true,
                     Do (Ident foobar, tmp.fsx (3,8--3,14) IsSynthetic=false),
                     DoBang
                       (Ident foobarBang, tmp.fsx (4,4--5,18) IsSynthetic=false),
                     tmp.fsx (3,8--5,18) IsSynthetic=false),
                  tmp.fsx (1,4--1,5) IsSynthetic=false, NoDebugPointAtLetBinding)],
              tmp.fsx (1,0--5,18) IsSynthetic=false)], PreXmlDocEmpty, [], None,
          tmp.fsx (1,0--5,18) IsSynthetic=false)], (true, true)))

DoBang starts at the keyword, while Do does not.

@nojaf
Copy link
Contributor Author

nojaf commented Jan 15, 2021

Closing in favor of #10881

@nojaf nojaf closed this Jan 15, 2021
@nojaf nojaf deleted the do-keyword-range branch May 31, 2022 07:04
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.

3 participants