fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

Range of `SynPat.Named` doesn't include `accessibility`

Open Booksbaum opened this issue 3 years ago • 3 comments

SynPat.Named(range=range) -> range doesn't include accessibility modifier (public, internal,public)

Repro steps

let private value = 42
//  ^^^^^^^^^^^^^ SynPat.Named(ident="value"; accessibility=Some(Private))
//          ^^^^^ range of SynPat.Named

Ast:

Named(
  SynIdent (value, None),
  false,
  Some Private,
  tmp.fsx (1,12-1,17)
)

-> Range covers columns 12 to 17 -> just value but not private

Expected behavior

Range of SynPat.Named includes private

Actual behavior

Range just covers identifier

Known workarounds

None?

SynAccess doesn't provide its range -> cannot combine ranges of elements inside SynPat.Named to get full range.

Maybe possible to infer by looking at parent and its SynPat.Named hole?
Or via source text: extract word in front of Named/its ident

Related information

SynPat.LongIdent includes SynAccess range:

let (private Some _) = None
//   ^^^^^^^^^^^^^^ SynPat.LongIdent
//   ^^^^^^^^^^^^^^ range of SynPat.LongIdent

(AST)



  • dotnet --version: 6.0.202
  • FCS: 41.0.4-preview.22181.2 & 41.0.3 & current dotnet/fsharp main branch (91fb77964b69d922b64ba62fcda4351731679588)

Booksbaum avatar May 08 '22 16:05 Booksbaum

@nojaf @auduchinok What are your thoughts on this?

dsyme avatar May 23 '22 15:05 dsyme

I didn't know it was possible to write a modifier inside parens like this and thought it should belonged to the containing binding ranges instead.

Looking at the tree, though, I see that an access modifier is being parsed as a part of a pattern (example 1, example 2, example 3). As things stand, it seems expected for a pattern to have the modifier inside its range, so it's in-line with changes to other keywords ranges across the parser that @nojaf and I were doing for some time.

auduchinok avatar May 23 '22 16:05 auduchinok

Hmm, yes it seems to be a thing that the modifier is part of the range. SynTypeDefn appears to have this as well. And then the parent SynModuleDecl.Types has the type keyword as well. So I guess this won't be a stretch that SynPat.Named had it also.

nojaf avatar May 24 '22 09:05 nojaf