Skip to content

Conversation

@nojaf
Copy link
Contributor

@nojaf nojaf commented Apr 29, 2021

Fixes #11487.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Thanks!

@cartermp
Copy link
Contributor

@nojaf some conflicts.

@nojaf nojaf force-pushed the include-range-attributes-signature-files branch from 519e853 to 251e2b7 Compare April 29, 2021 19:08
@nojaf
Copy link
Contributor Author

nojaf commented Apr 29, 2021

Resolved!

@cartermp cartermp merged commit fc42e0b into dotnet:main Apr 30, 2021
@nojaf nojaf deleted the include-range-attributes-signature-files branch April 30, 2021 05:00
@auduchinok
Copy link
Member

This PR breaks ranges for type signatures that don't have attributes. For the following signature the range now starts at R instead of type:

type R =
    { F: int }

I'm working on a fix.

@nojaf
Copy link
Contributor Author

nojaf commented May 18, 2021

Would that not be consist with the range you'd get when it is inside an implementation file?
I thought keywords were not supposed to be part of the range.

@auduchinok
Copy link
Member

So you're suggesting that there should be this difference?

type R1 = // type is not included
    { F: int }

[<A>]
type R = // type is included
    { F: int }

It looks very wrong to me.

Would that not be consist with the range you'd get when it is inside an implementation file?

IIRC implementation files should already contain them. If not, we should fix it.

@nojaf
Copy link
Contributor Author

nojaf commented May 18, 2021

I'd be in favor of having them included, just wanted to point out that in FCS 39, the type keyword is not included in TypeDefn.
See example.

type R = { Foo: int }
ImplFile
  (ParsedImplFileInput
     ("tmp.fsx", true, QualifiedNameOfFile Tmp$fsx, [], [],
      [SynModuleOrNamespace
         ([Tmp], false, AnonModule,
          [Types
             ([TypeDefn
                 (ComponentInfo
                    ([], [], [], [R],
                     PreXmlDoc ((1,6), FSharp.Compiler.XmlDoc+XmlDocCollector),
                     false, None, tmp.fsx (1,5--1,6) IsSynthetic=false),
                  Simple
                    (Record
                       (None,
                        [Field
                           ([], false, Some Foo,
                            LongIdent (LongIdentWithDots ([int], [])), false,
                            PreXmlDoc ((1,14), FSharp.Compiler.XmlDoc+XmlDocCollector),
                            None, tmp.fsx (1,11--1,19) IsSynthetic=false)],
                        tmp.fsx (1,9--1,21) IsSynthetic=false),
                     tmp.fsx (1,9--1,21) IsSynthetic=false), [],
                  tmp.fsx (1,5--1,21) IsSynthetic=false)],
              tmp.fsx (1,0--1,21) IsSynthetic=false)], PreXmlDocEmpty, [], None,
          tmp.fsx (1,0--1,21) IsSynthetic=false)], (true, true)))

@auduchinok
Copy link
Member

auduchinok commented May 18, 2021

Ah, I see where I wasn't clear.

We use SynModuleDecl.Types.Range for looking at where types groups start, and this is the part where it has regressed, not individual type signature ranges. In your tree dump the group does indeed contain type.

@nojaf
Copy link
Contributor Author

nojaf commented May 18, 2021

Ah ok, now I understand what regressed. Thanks for explaining.

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.

Attributes not included in the range of the parent node (signature files)

4 participants