Skip to content

Correct ranges for nested modules in signature files. #12585

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 10, 2022
Merged

Correct ranges for nested modules in signature files. #12585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
313af5a
Correct ranges for nested modules in signature files.
nojaf Jan 7, 2022
cfc7f6c
Use typeNameInfo and mWithKwd for range of SynTypeDefnSig without mem…
nojaf Jan 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/fsharp/pars.fsy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,11 +816,10 @@ moduleSpfn:
_xmlDoc.MarkAsInvalid()
let attrs = $1 @ cas
let mTc =
let keywordM = rhs parseState 3
(keywordM, attrs) ||> unionRangeWithListBy (fun (a: SynAttributeList) -> a.Range) |> unionRanges range
(d3, attrs) ||> unionRangeWithListBy (fun (a: SynAttributeList) -> a.Range) |> unionRanges range
let xmlDoc = grabXmlDoc(parseState, $1, 1)
let tc = (SynTypeDefnSig(SynComponentInfo(attrs, a, cs, b, xmlDoc, d, d2, d3), equalsRange, typeRepr, withKeyword, members, mTc))
let m = (mTc, $5) ||> unionRangeWithListBy (fun (a: SynTypeDefnSig) -> a.Range)
let m = (mTc, $5) ||> unionRangeWithListBy (fun (a: SynTypeDefnSig) -> a.Range) |> unionRanges (rhs parseState 3)
SynModuleSigDecl.Types (tc :: $5, m) }

| opt_attributes opt_declVisibility exconSpfn
Expand Down Expand Up @@ -895,7 +894,16 @@ tyconSpfn:
$3 lhsm $1 (Some mEquals) }
| typeNameInfo opt_classSpfn
{ let mWithKwd, members = $2
SynTypeDefnSig($1, None, SynTypeDefnSigRepr.Simple (SynTypeDefnSimpleRepr.None (lhs parseState), lhs parseState), mWithKwd, members, lhs parseState) }
let m =
match members with
| [] ->
match mWithKwd with
| None -> rhs parseState 1
| Some mWithKwd -> unionRanges (rhs parseState 1) mWithKwd
| decls ->
let (SynComponentInfo(range=start)) = $1
(start, decls) ||> unionRangeWithListBy (fun (s: SynMemberSig) -> s.Range)
SynTypeDefnSig($1, None, SynTypeDefnSigRepr.Simple (SynTypeDefnSimpleRepr.None m, m), mWithKwd, members, m) }


/* The right-hand-side of a type definition in a signature */
Expand Down Expand Up @@ -930,8 +938,14 @@ tyconSpfnRhs:
SynTypeDefnSig(nameInfo, mEquals, SynTypeDefnSigRepr.Simple ($1, $1.Range), None, augmentation, mWhole)) }

| tyconClassSpfn
{ let objectModelRange = lhs parseState
let needsCheck, (kind, decls) = $1
{ let needsCheck, (kind, decls) = $1
let objectModelRange =
match decls with
| [] -> lhs parseState
Copy link
Member

@auduchinok auduchinok Jan 8, 2022

Choose a reason for hiding this comment

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

@nojaf Would it be hard to extract successfully ranges here too? Sorry, I should've added comment here too. 🙂

Copy link
Contributor Author

@nojaf nojaf Jan 8, 2022

Choose a reason for hiding this comment

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

Would that be replacing it with rhs parseState 1?

Copy link
Member

@auduchinok auduchinok Jan 8, 2022

Choose a reason for hiding this comment

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

From a glance, it seems it may require passing parsed ranges via tyconClassSpfn and other rules. Otherwise recovered ranges may still span further than they should. Not sure if you'd want to do that as a part of this PR. 🙂

It'd be great to do this systematically, the same as we've been fixing ranges of the things.

Copy link
Contributor Author

@nojaf nojaf Jan 8, 2022

Choose a reason for hiding this comment

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

Hmm, I see what you mean. I'm not going to push my luck in this current PR 😅.
I'll let it be while the PR is still green.

Copy link
Contributor

@KevinRansom KevinRansom Jan 10, 2022

Choose a reason for hiding this comment

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

@nojaf , I will merge this as is, can you prepare a PR with the discussed modification and additional tests to cover that additional scenario please?

Thanks

Kevin

Copy link
Contributor Author

@nojaf nojaf Jan 11, 2022

Choose a reason for hiding this comment

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

Hey Kevin, thanks for merging this.
I can circle back to this in the future, @auduchinok any specific examples you can think of to cover this?

Copy link
Member

@auduchinok auduchinok Jan 12, 2022

Choose a reason for hiding this comment

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

@nojaf Nope, I couldn't easily find an example. I'm going to look at members parser recovery later, and maybe I'll find an example. What happened in #12224 was it started returning intersecting ranges only after a better recovery was added to match clauses and patterns parsing, so it could probably happen here too.

| decls ->
let start = mkSynRange parseState.ResultStartPosition parseState.ResultStartPosition
(start, decls) ||> unionRangeWithListBy (fun (s: SynMemberSig) -> s.Range)

(fun nameRange nameInfo mEquals augmentation ->
if needsCheck && isNil decls then
reportParseErrorAt nameRange (FSComp.SR.parsEmptyTypeDefinition())
Expand Down
120 changes: 108 additions & 12 deletions tests/service/Symbols.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1138,8 +1138,9 @@ type MyRecord =

match parseResults with
| ParsedInput.SigFile (ParsedSigFileInput (modules = [
SynModuleOrNamespaceSig(decls = [SynModuleSigDecl.Types(types = [SynTypeDefnSig.SynTypeDefnSig(range = r)])]) ])) ->
assertRange (2, 0) (4, 30) r
SynModuleOrNamespaceSig(decls = [SynModuleSigDecl.Types([SynTypeDefnSig.SynTypeDefnSig(range=mSynTypeDefnSig)], mTypes)]) ])) ->
assertRange (2, 0) (4, 30) mTypes
assertRange (2, 5) (4, 30) mSynTypeDefnSig
| _ -> Assert.Fail "Could not get valid AST"

[<Test>]
Expand All @@ -1154,8 +1155,9 @@ type MyRecord =

match parseResults with
| ParsedInput.SigFile (ParsedSigFileInput (modules = [
SynModuleOrNamespaceSig(decls = [SynModuleSigDecl.Types(types = [SynTypeDefnSig.SynTypeDefnSig(range = r)])]) ])) ->
assertRange (2, 0) (5, 30) r
SynModuleOrNamespaceSig(decls = [SynModuleSigDecl.Types([SynTypeDefnSig.SynTypeDefnSig(range=mSynTypeDefnSig)], mTypes)]) ])) ->
assertRange (2, 0) (5, 30) mTypes
assertRange (2, 5) (5, 30) mSynTypeDefnSig
| _ -> Assert.Fail "Could not get valid AST"

[<Test>]
Expand All @@ -1168,8 +1170,9 @@ type MyFunction =

match parseResults with
| ParsedInput.SigFile (ParsedSigFileInput (modules = [
SynModuleOrNamespaceSig(decls = [SynModuleSigDecl.Types(types = [SynTypeDefnSig.SynTypeDefnSig(range = r)])]) ])) ->
assertRange (2, 0) (3, 29) r
SynModuleOrNamespaceSig(decls = [SynModuleSigDecl.Types([SynTypeDefnSig.SynTypeDefnSig(range=mSynTypeDefnSig)], mTypes) ]) ])) ->
assertRange (2, 0) (3, 29) mTypes
assertRange (2, 5) (3, 29) mSynTypeDefnSig
| _ -> Assert.Fail "Could not get valid AST"

[<Test>]
Expand All @@ -1183,8 +1186,9 @@ type SomeCollection with

match parseResults with
| ParsedInput.SigFile (ParsedSigFileInput (modules = [
SynModuleOrNamespaceSig(decls = [SynModuleSigDecl.Types(types = [SynTypeDefnSig.SynTypeDefnSig(range = r)])]) ])) ->
assertRange (2, 0) (4, 37) r
SynModuleOrNamespaceSig(decls = [SynModuleSigDecl.Types([SynTypeDefnSig.SynTypeDefnSig(range=mSynTypeDefnSig)], mTypes)]) ])) ->
assertRange (2, 0) (4, 37) mTypes
assertRange (2, 5) (4, 37) mSynTypeDefnSig
| _ -> Assert.Fail "Could not get valid AST"

[<Test>]
Expand Down Expand Up @@ -1227,13 +1231,13 @@ and [<CustomEquality>] Bang =

match parseResults with
| ParsedInput.SigFile (ParsedSigFileInput (modules = [
SynModuleOrNamespaceSig(decls = [SynModuleSigDecl.Types(types = [
SynModuleOrNamespaceSig(decls = [SynModuleSigDecl.Types([
SynTypeDefnSig.SynTypeDefnSig(range = r1)
SynTypeDefnSig.SynTypeDefnSig(range = r2)
]) as t]) ])) ->
assertRange (4, 0) (5, 9) r1
], mTypes)]) ])) ->
assertRange (4, 5) (5, 9) r1
assertRange (7, 4) (12, 42) r2
assertRange (4, 0) (12, 42) t.Range
assertRange (4, 0) (12, 42) mTypes
| _ -> Assert.Fail "Could not get valid AST"

[<Test>]
Expand Down Expand Up @@ -1717,6 +1721,98 @@ module X =
assertRange (4, 9) (4, 10) equalsM
| _ -> Assert.Fail "Could not get valid AST"

[<Test>]
let ``Range of nested module in signature file should end at the last SynModuleSigDecl`` () =
let parseResults =
getParseResultsOfSignatureFile
"""namespace Microsoft.FSharp.Core

open System
open System.Collections.Generic
open Microsoft.FSharp.Core
open Microsoft.FSharp.Collections
open System.Collections


module Tuple =

type Tuple<'T1,'T2,'T3,'T4> =
interface IStructuralEquatable
interface IStructuralComparable
interface IComparable
new : 'T1 * 'T2 * 'T3 * 'T4 -> Tuple<'T1,'T2,'T3,'T4>
member Item1 : 'T1 with get
member Item2 : 'T2 with get
member Item3 : 'T3 with get
member Item4 : 'T4 with get


module Choice =

/// <summary>Helper types for active patterns with 6 choices.</summary>
[<StructuralEquality; StructuralComparison>]
[<CompiledName("FSharpChoice`6")>]
type Choice<'T1,'T2,'T3,'T4,'T5,'T6> =
/// <summary>Choice 1 of 6 choices</summary>
| Choice1Of6 of 'T1
/// <summary>Choice 2 of 6 choices</summary>
| Choice2Of6 of 'T2
/// <summary>Choice 3 of 6 choices</summary>
| Choice3Of6 of 'T3
/// <summary>Choice 4 of 6 choices</summary>
| Choice4Of6 of 'T4
/// <summary>Choice 5 of 6 choices</summary>
| Choice5Of6 of 'T5
/// <summary>Choice 6 of 6 choices</summary>
| Choice6Of6 of 'T6



/// <summary>Basic F# Operators. This module is automatically opened in all F# code.</summary>
[<AutoOpen>]
module Operators =

type ``[,]``<'T> with
[<CompiledName("Length1")>]
/// <summary>Get the length of an array in the first dimension </summary>
member Length1 : int
[<CompiledName("Length2")>]
/// <summary>Get the length of the array in the second dimension </summary>
member Length2 : int
[<CompiledName("Base1")>]
/// <summary>Get the lower bound of the array in the first dimension </summary>
member Base1 : int
[<CompiledName("Base2")>]
/// <summary>Get the lower bound of the array in the second dimension </summary>
member Base2 : int
"""

match parseResults with
| ParsedInput.SigFile (ParsedSigFileInput (modules = [ SynModuleOrNamespaceSig(decls = [
SynModuleSigDecl.Open _
SynModuleSigDecl.Open _
SynModuleSigDecl.Open _
SynModuleSigDecl.Open _
SynModuleSigDecl.Open _
SynModuleSigDecl.NestedModule(range=mTupleModule; moduleDecls=[ SynModuleSigDecl.Types([
SynTypeDefnSig(typeRepr=SynTypeDefnSigRepr.ObjectModel(range=mTupleObjectModel); range=mTupleType)
], mTupleTypes) ])
SynModuleSigDecl.NestedModule(range=mChoiceModule)
SynModuleSigDecl.NestedModule(range=mOperatorsModule; moduleDecls=[ SynModuleSigDecl.Types([
SynTypeDefnSig(typeRepr=SynTypeDefnSigRepr.Simple(range=mAugmentationSimple); range=mAugmentation)
], mOperatorsTypes) ])
]) ])) ->
assertRange (10, 0) (20, 35) mTupleModule
assertRange (12, 4) (20, 35) mTupleTypes
assertRange (12, 9) (20, 35) mTupleType
assertRange (13, 8) (20, 35) mTupleObjectModel
assertRange (23, 0) (40, 25) mChoiceModule
assertRange (45, 0) (60, 26) mOperatorsModule
assertRange (48, 4) (60, 26) mOperatorsTypes
assertRange (48, 9) (60, 26) mAugmentation
assertRange (48, 9) (60, 26) mAugmentationSimple
| _ -> Assert.Fail "Could not get valid AST"

module SynBindings =
[<Test>]
let ``Range of attribute should be included in SynModuleDecl.Let`` () =
Expand Down