Skip to content

Commit

Permalink
Disallow abstract member with access modifiers in sig file (#17802)
Browse files Browse the repository at this point in the history 
* Disallow abstract member with access modifiers in sig file

* release note

* format

* fix

* fix test

* use access modifier range to show error

* update tests

* update tests

* show both FS0531 and FS0561

---------

Co-authored-by: ijklam <[email protected]>
Co-authored-by: Kevin Ransom (msft) <[email protected]>
  • Loading branch information
@ijklam @KevinRansom
3 people authored Oct 24, 2024
1 parent ee8af8a commit 8b0900c
Show file tree
Hide file tree
Showing 7 changed files with 82 additions and 28 deletions.
3 changes: 2 additions & 1 deletion docs/release-notes/.FSharp.Compiler.Service/9.0.200.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
* Fix false negatives for passing null to "obj" arguments. Only "obj | null" can now subsume any type ([PR #17757](https://github.com/dotnet/fsharp/pull/17757))
* Fix internal error when calling 'AddSingleton' and other overloads only differing in generic arity ([PR #17804](https://github.com/dotnet/fsharp/pull/17804))
* Fix extension methods support for non-reference system assemblies ([PR #17799](https://github.com/dotnet/fsharp/pull/17799))
* Ensure `frameworkTcImportsCache` mutations are thread-safe. ([PR #17795](https://github.com/dotnet/fsharp/pull/17795))
* Ensure `frameworkTcImportsCache` mutations are threadsafe. ([PR #17795](https://github.com/dotnet/fsharp/pull/17795))
* Disallow abstract member with access modifiers in sig file. ([PR #17802](https://github.com/dotnet/fsharp/pull/17802))
* Fix concurrency issue in `ILPreTypeDefImpl` ([PR #17812](https://github.com/dotnet/fsharp/pull/17812))
* Fix nullness inference for member val and other OO scenarios ([PR #17845](https://github.com/dotnet/fsharp/pull/17845))
* Fix internal error when analyzing incomplete inherit member ([PR #17905](https://github.com/dotnet/fsharp/pull/17905))
Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Service/FSharpSource.fsi
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type internal FSharpSource =
abstract TimeStamp: DateTime

/// Gets the internal text container. Text may be on-disk, in a stream, or a source text.
abstract internal GetTextContainer: unit -> Async<TextContainer>
abstract GetTextContainer: unit -> Async<TextContainer>

/// Creates a FSharpSource from disk. Only used internally.
static member internal CreateFromFile: filePath: string -> FSharpSource
Expand Down
8 changes: 8 additions & 0 deletions src/Compiler/SyntaxTree/ParseHelpers.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1229,3 +1229,11 @@ let mkValField
mkSynField parseState idOpt typ isMutable access attribs mStaticOpt rangeStart (Some leadingKeyword)

SynMemberDefn.ValField(field, field.Range)

let leadingKeywordIsAbstract =
function
| SynLeadingKeyword.Abstract _
| SynLeadingKeyword.AbstractMember _
| SynLeadingKeyword.StaticAbstract _
| SynLeadingKeyword.StaticAbstractMember _ -> true
| _ -> false
2 changes: 2 additions & 0 deletions src/Compiler/SyntaxTree/ParseHelpers.fsi
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,3 +302,5 @@ val mkSynField:
rangeStart: range ->
leadingKeyword: SynLeadingKeyword option ->
SynField

val leadingKeywordIsAbstract: SynLeadingKeyword -> bool
23 changes: 15 additions & 8 deletions src/Compiler/pars.fsy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -985,8 +985,12 @@ classMemberSpfn:
match optLiteralValue with
| None -> m
| Some e -> unionRanges m e.Range

let flags, leadingKeyword = $3
if leadingKeywordIsAbstract leadingKeyword then
[ $2; $5; getterAccess; setterAccess ]
|> List.iter (function None -> () | Some access -> errorR(Error(FSComp.SR.parsAccessibilityModsIllegalForAbstract(), access.Range)))

let flags = flags (getSetAdjuster arity)
let trivia = { LeadingKeyword = leadingKeyword; InlineKeyword = $4; WithKeyword = mWith; EqualsRange = mEquals }
let valSpfn = SynValSig($1, id, explicitValTyparDecls, ty, arity, isInline, false, doc, vis2, optLiteralValue, mWhole, trivia)
Expand Down Expand Up @@ -2046,21 +2050,24 @@ classDefnMember:
let ty = SynType.FromParseError(mInterface.EndRange)
[ SynMemberDefn.Interface(ty, None, None, rhs2 parseState 1 3) ] }

| opt_attributes opt_access abstractMemberFlags opt_inline nameop opt_explicitValTyparDecls COLON topTypeWithTypeConstraints classMemberSpfnGetSet opt_ODECLEND
{ let ty, arity = $8
let isInline, doc, id, explicitValTyparDecls = (Option.isSome $4), grabXmlDoc(parseState, $1, 1), $5, $6
let mWith, (getSet, getSetRangeOpt, getterAccess, setterAccess) = $9
| opt_attributes opt_access abstractMemberFlags opt_access opt_inline nameop opt_explicitValTyparDecls COLON topTypeWithTypeConstraints classMemberSpfnGetSet opt_ODECLEND
{ if Option.isSome $2 then errorR(Error(FSComp.SR.parsVisibilityDeclarationsShouldComePriorToIdentifier(), rhs parseState 2))
let ty, arity = $9
let isInline, doc, id, explicitValTyparDecls = (Option.isSome $5), grabXmlDoc(parseState, $1, 1), $6, $7
let mWith, (getSet, getSetRangeOpt, getterAccess, setterAccess) = $10
let getSetAdjuster arity = match arity, getSet with SynValInfo([], _), SynMemberKind.Member -> SynMemberKind.PropertyGet | _ -> getSet
let mWhole =
let m = rhs parseState 1
match getSetRangeOpt with
| None -> unionRanges m ty.Range
| Some gs -> unionRanges m gs.Range
|> unionRangeWithXmlDoc doc
if Option.isSome $2 || Option.isSome getterAccess || Option.isSome setterAccess
then errorR(Error(FSComp.SR.parsAccessibilityModsIllegalForAbstract(), mWhole))

[ $2; $4; getterAccess; setterAccess ]
|> List.iter (function None -> () | Some access -> errorR(Error(FSComp.SR.parsAccessibilityModsIllegalForAbstract(), access.Range)))

let mkFlags, leadingKeyword = $3
let trivia = { LeadingKeyword = leadingKeyword; InlineKeyword = $4; WithKeyword = mWith; EqualsRange = None }
let trivia = { LeadingKeyword = leadingKeyword; InlineKeyword = $5; WithKeyword = mWith; EqualsRange = None }
let vis2 = SynValSigAccess.Single(None)
let valSpfn = SynValSig($1, id, explicitValTyparDecls, ty, arity, isInline, false, doc, vis2, None, mWhole, trivia)
let trivia: SynMemberDefnAbstractSlotTrivia = { GetSetKeywords = getSetRangeOpt }
Expand Down
59 changes: 47 additions & 12 deletions ...ce/BasicGrammarElements/AccessibilityAnnotations/PermittedLocations/PermittedLocations.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,15 @@ module AccessibilityAnnotations_PermittedLocations =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 561, Line 18, Col 5, Line 18, Col 62, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 561, Line 19, Col 5, Line 19, Col 62, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 561, Line 20, Col 5, Line 20, Col 62, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 10, Line 21, Col 14, Line 21, Col 20, "Unexpected keyword 'public' in member definition. Expected identifier, '(', '(*)' or other token.")
(Error 561, Line 21, Col 14, Line 22, Col 62, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 10, Line 23, Col 14, Line 23, Col 22, "Unexpected keyword 'internal' in member definition. Expected identifier, '(', '(*)' or other token.")
(Error 531, Line 18, Col 5, Line 18, Col 11, "Accessibility modifiers should come immediately prior to the identifier naming a construct")
(Error 561, Line 18, Col 5, Line 18, Col 11, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 531, Line 19, Col 5, Line 19, Col 12, "Accessibility modifiers should come immediately prior to the identifier naming a construct")
(Error 561, Line 19, Col 5, Line 19, Col 12, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 531, Line 20, Col 5, Line 20, Col 13, "Accessibility modifiers should come immediately prior to the identifier naming a construct")
(Error 561, Line 20, Col 5, Line 20, Col 13, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 561, Line 21, Col 14, Line 21, Col 20, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 561, Line 22, Col 14, Line 22, Col 21, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 561, Line 23, Col 14, Line 23, Col 22, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
]

// SOURCE=E_accessibilityOnInterface01.fs SCFLAGS="--test:ErrorRanges" # E_accessibilityOnInterface01.fs
Expand All @@ -42,7 +45,8 @@ module AccessibilityAnnotations_PermittedLocations =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 561, Line 13, Col 5, Line 13, Col 67, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 531, Line 13, Col 5, Line 13, Col 11, "Accessibility modifiers should come immediately prior to the identifier naming a construct")
(Error 561, Line 13, Col 5, Line 13, Col 11, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
]

// SOURCE=E_accessibilityOnInterface02.fs SCFLAGS="--test:ErrorRanges" # E_accessibilityOnInterface02.fs
Expand All @@ -52,7 +56,8 @@ module AccessibilityAnnotations_PermittedLocations =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 561, Line 15, Col 5, Line 15, Col 68, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 531, Line 15, Col 5, Line 15, Col 12, "Accessibility modifiers should come immediately prior to the identifier naming a construct")
(Error 561, Line 15, Col 5, Line 15, Col 12, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
]

// SOURCE=E_accessibilityOnInterface03.fs SCFLAGS="--test:ErrorRanges" # E_accessibilityOnInterface03.fs
Expand All @@ -62,7 +67,8 @@ module AccessibilityAnnotations_PermittedLocations =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 561, Line 15, Col 5, Line 15, Col 69, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 531, Line 15, Col 5, Line 15, Col 13, "Accessibility modifiers should come immediately prior to the identifier naming a construct")
(Error 561, Line 15, Col 5, Line 15, Col 13, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
]

// SOURCE=E_accessibilityOnInterface04.fs SCFLAGS="--test:ErrorRanges" # E_accessibilityOnInterface04.fs
Expand All @@ -72,7 +78,7 @@ module AccessibilityAnnotations_PermittedLocations =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 10, Line 15, Col 14, Line 15, Col 20, "Unexpected keyword 'public' in member definition. Expected identifier, '(', '(*)' or other token.")
(Error 561, Line 15, Col 14, Line 15, Col 20, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
]

// SOURCE=E_accessibilityOnInterface05.fs SCFLAGS="--test:ErrorRanges" # E_accessibilityOnInterface05.fs
Expand All @@ -82,7 +88,7 @@ module AccessibilityAnnotations_PermittedLocations =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 10, Line 15, Col 14, Line 15, Col 21, "Unexpected keyword 'private' in member definition. Expected identifier, '(', '(*)' or other token.")
(Error 561, Line 15, Col 14, Line 15, Col 21, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
]

// SOURCE=E_accessibilityOnInterface06.fs SCFLAGS="--test:ErrorRanges" # E_accessibilityOnInterface06.fs
Expand All @@ -92,7 +98,7 @@ module AccessibilityAnnotations_PermittedLocations =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 10, Line 15, Col 14, Line 15, Col 22, "Unexpected keyword 'internal' in member definition. Expected identifier, '(', '(*)' or other token.")
(Error 561, Line 15, Col 14, Line 15, Col 22, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
]

// SOURCE=E_accessibilityOnRecords.fs SCFLAGS="--test:ErrorRanges" # E_accessibilityOnRecords.fs
Expand Down Expand Up @@ -160,3 +166,32 @@ module AccessibilityAnnotations_PermittedLocations =
|> withDiagnostics [
(Error 531, Line 8, Col 13, Line 8, Col 20, "Accessibility modifiers should come immediately prior to the identifier naming a construct")
]

[<Fact>]
let ``Signature File Test: abstract member cannot have access modifiers`` () =
Fsi """module Program
type A =
abstract internal B: int ->int
abstract member internal E: int ->int
abstract member C: int with internal get, private set
abstract internal D: int with get, set
static abstract internal B2: int ->int
static abstract member internal E2: int ->int
static abstract member C2: int with internal get, private set
static abstract internal D2: int with get, set"""
|> withOptions ["--nowarn:3535"]
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 0561, Line 4, Col 14, Line 4, Col 22, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 5, Col 21, Line 5, Col 29, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 6, Col 33, Line 6, Col 41, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 6, Col 47, Line 6, Col 54, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 7, Col 14, Line 7, Col 22, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 8, Col 21, Line 8, Col 29, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 9, Col 28, Line 9, Col 36, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 10, Col 41, Line 10, Col 49, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 10, Col 55, Line 10, Col 62, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 11, Col 21, Line 11, Col 29, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
]
13 changes: 7 additions & 6 deletions ...ammarElements/MemberDefinitions/MethodsAndProperties/AutoPropsWithModifierBeforeGetSet.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,12 @@ let ``Abstract Properties Test: access modifiers are not allowed`` () =
|> typecheck
|> shouldFail
|> withDiagnostics [
(Error 0561, Line 6, Col 5, Line 6, Col 51, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 8, Col 5, Line 8, Col 51, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 10, Col 5, Line 10, Col 60, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 12, Col 5, Line 12, Col 46, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 14, Col 5, Line 14, Col 46, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 6, Col 34, Line 6, Col 42, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 8, Col 39, Line 8, Col 47, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 10, Col 34, Line 10, Col 42, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 10, Col 48, Line 10, Col 56, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 12, Col 34, Line 12, Col 42, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
(Error 0561, Line 14, Col 34, Line 14, Col 42, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
]

[<Fact>]
Expand All @@ -132,7 +133,7 @@ type A =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 240, Line 1, Col 1, Line 9, Col 42, "The signature file 'Program' does not have a corresponding implementation file. If an implementation file exists then check the 'module' and 'namespace' declarations in the signature and implementation files match.")
(Error 0561, Line 9, Col 31, Line 9, Col 38, "Accessibility modifiers are not allowed on this member. Abstract slots always have the same visibility as the enclosing type.")
]

[<Fact>]
Expand Down

0 comments on commit 8b0900c

Please sign in to comment.