From 8b0900c78e0fa1521ab06af06af410d8f7400414 Mon Sep 17 00:00:00 2001 From: ijklam <43789618+ijklam@users.noreply.github.com> Date: Thu, 24 Oct 2024 20:10:10 +0800 Subject: [PATCH] Disallow abstract member with access modifiers in sig file (#17802) * 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 <43789618+Tangent-90@users.noreply.github.com> Co-authored-by: Kevin Ransom (msft) --- .../.FSharp.Compiler.Service/9.0.200.md | 3 +- src/Compiler/Service/FSharpSource.fsi | 2 +- src/Compiler/SyntaxTree/ParseHelpers.fs | 8 +++ src/Compiler/SyntaxTree/ParseHelpers.fsi | 2 + src/Compiler/pars.fsy | 23 +++++--- .../PermittedLocations/PermittedLocations.fs | 59 +++++++++++++++---- .../AutoPropsWithModifierBeforeGetSet.fs | 13 ++-- 7 files changed, 82 insertions(+), 28 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/9.0.200.md b/docs/release-notes/.FSharp.Compiler.Service/9.0.200.md index 043b2b67178..b906a5ffec9 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/9.0.200.md +++ b/docs/release-notes/.FSharp.Compiler.Service/9.0.200.md @@ -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)) diff --git a/src/Compiler/Service/FSharpSource.fsi b/src/Compiler/Service/FSharpSource.fsi index ca272a51fef..6bdabbdedf1 100644 --- a/src/Compiler/Service/FSharpSource.fsi +++ b/src/Compiler/Service/FSharpSource.fsi @@ -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 + abstract GetTextContainer: unit -> Async /// Creates a FSharpSource from disk. Only used internally. static member internal CreateFromFile: filePath: string -> FSharpSource diff --git a/src/Compiler/SyntaxTree/ParseHelpers.fs b/src/Compiler/SyntaxTree/ParseHelpers.fs index c11647ab0a5..59fb645bd0a 100644 --- a/src/Compiler/SyntaxTree/ParseHelpers.fs +++ b/src/Compiler/SyntaxTree/ParseHelpers.fs @@ -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 diff --git a/src/Compiler/SyntaxTree/ParseHelpers.fsi b/src/Compiler/SyntaxTree/ParseHelpers.fsi index c98e7897a32..f769b14a864 100644 --- a/src/Compiler/SyntaxTree/ParseHelpers.fsi +++ b/src/Compiler/SyntaxTree/ParseHelpers.fsi @@ -302,3 +302,5 @@ val mkSynField: rangeStart: range -> leadingKeyword: SynLeadingKeyword option -> SynField + +val leadingKeywordIsAbstract: SynLeadingKeyword -> bool diff --git a/src/Compiler/pars.fsy b/src/Compiler/pars.fsy index 698527f24d9..0e56092fe54 100644 --- a/src/Compiler/pars.fsy +++ b/src/Compiler/pars.fsy @@ -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) @@ -2046,10 +2050,11 @@ 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 @@ -2057,10 +2062,12 @@ classDefnMember: | 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 } diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/AccessibilityAnnotations/PermittedLocations/PermittedLocations.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/AccessibilityAnnotations/PermittedLocations/PermittedLocations.fs index 68eb881ff7f..7f4c02ab56f 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/AccessibilityAnnotations/PermittedLocations/PermittedLocations.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/AccessibilityAnnotations/PermittedLocations/PermittedLocations.fs @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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") ] + + [] + 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.") + ] diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/MemberDefinitions/MethodsAndProperties/AutoPropsWithModifierBeforeGetSet.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/MemberDefinitions/MethodsAndProperties/AutoPropsWithModifierBeforeGetSet.fs index 676746d96f5..7e7cdff06cc 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/MemberDefinitions/MethodsAndProperties/AutoPropsWithModifierBeforeGetSet.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/MemberDefinitions/MethodsAndProperties/AutoPropsWithModifierBeforeGetSet.fs @@ -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.") ] [] @@ -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.") ] []