From dd3b52cc638150c943b4cb4c16237f4142d085ac Mon Sep 17 00:00:00 2001 From: KevinRansom Date: Tue, 5 Mar 2024 00:37:08 -0800 Subject: [PATCH 1/5] Fix #13926 - BadImageFormatException : Bad IL format when using base --- .../.FSharp.Compiler.Service/8.0.300.md | 1 + docs/release-notes/.Language/8.0.md | 6 +- src/Compiler/AbstractIL/il.fs | 3 +- src/Compiler/Checking/PostInferenceChecks.fs | 12 ++-- src/Compiler/FSharp.Compiler.Service.fsproj | 21 +++--- .../ImportDeclarations/ImportDeclarations.fs | 71 +++++++++++++++++++ .../ErrorMessages/SuggestionsTests.fs | 6 +- .../FSharp.Compiler.ComponentTests.fsproj | 19 +++-- 8 files changed, 108 insertions(+), 31 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md index 83a1b23aa9f..35cb6319ddc 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md @@ -15,6 +15,7 @@ * Fix release inline optimization, which leads to MethodAccessException if used with `assembly:InternalsVisibleTo`` attribute. ([Issue #16105](https://github.com/dotnet/fsharp/issues/16105), ([PR #16737](https://github.com/dotnet/fsharp/pull/16737)) * Enforce AttributeTargets on let values and functions. ([PR #16692](https://github.com/dotnet/fsharp/pull/16692)) * Enforce AttributeTargets on union case declarations. ([PR #16764](https://github.com/dotnet/fsharp/pull/16764)) +* Disallow using base to invoke an abstract base method. ([Issue #13926](https://github.com/dotnet/fsharp/issues/13926), [PR #16773](https://github.com/dotnet/fsharp/pull/16773)) ### Added diff --git a/docs/release-notes/.Language/8.0.md b/docs/release-notes/.Language/8.0.md index ad829f0a6b9..f44c3df1f39 100644 --- a/docs/release-notes/.Language/8.0.md +++ b/docs/release-notes/.Language/8.0.md @@ -1,3 +1,7 @@ -### Added +### Fixed + +* Disallow using base to invoke an abstract base method. . ([Issue #13926](https://github.com/dotnet/fsharp/issues/13926), [PR #16773](https://github.com/dotnet/fsharp/pull/16773)) + +### Added * `while!` ([Language suggestion #1038](https://github.com/fsharp/fslang-suggestions/issues/1038), [PR #14238](https://github.com/dotnet/fsharp/pull/14238)) \ No newline at end of file diff --git a/src/Compiler/AbstractIL/il.fs b/src/Compiler/AbstractIL/il.fs index 7f303178553..43ac3e930a9 100644 --- a/src/Compiler/AbstractIL/il.fs +++ b/src/Compiler/AbstractIL/il.fs @@ -1910,6 +1910,8 @@ type ILMethodDef metadataIndex: int32 ) = + let _x = () + new(name, attributes, implAttributes, callingConv, parameters, ret, body, isEntryPoint, genericParams, securityDecls, customAttrs) = ILMethodDef( name, @@ -5641,7 +5643,6 @@ let resolveILMethodRefWithRescope r (td: ILTypeDef) (mref: ILMethodRef) = |> List.filter (fun md -> mref.CallingConv = md.CallingConv && - // REVIEW: this uses equality on ILType. For CMOD_OPTIONAL this is not going to be correct (md.Parameters, argTypes) ||> List.lengthsEqAndForall2 (fun p1 p2 -> r p1.Type = p2) && diff --git a/src/Compiler/Checking/PostInferenceChecks.fs b/src/Compiler/Checking/PostInferenceChecks.fs index e15d9e9463f..852be12aff0 100644 --- a/src/Compiler/Checking/PostInferenceChecks.fs +++ b/src/Compiler/Checking/PostInferenceChecks.fs @@ -1307,14 +1307,14 @@ and CheckILBaseCall cenv env (ilMethRef, enclTypeInst, methInst, retTypes, tyarg match tryTcrefOfAppTy g baseVal.Type with | ValueSome tcref when tcref.IsILTycon -> try - // This is awkward - we have to explicitly re-resolve back to the IL metadata to determine if the method is abstract. - // We believe this may be fragile in some situations, since we are using the Abstract IL code to compare - // type equality, and it would be much better to remove any F# dependency on that implementation of IL type - // equality. It would be better to make this check in tc.fs when we have the Abstract IL metadata for the method to hand. - let mdef = resolveILMethodRef tcref.ILTyconRawMetadata ilMethRef + let mdef = + match tcref.ILTyconInfo with + | TILObjectReprData(scoref, _, _) -> + resolveILMethodRefWithRescope (rescopeILType scoref) tcref.ILTyconRawMetadata ilMethRef + if mdef.IsAbstract then errorR(Error(FSComp.SR.tcCannotCallAbstractBaseMember(mdef.Name), m)) - with _ -> () // defensive coding + with _ -> () | _ -> () CheckTypeInstNoByrefs cenv env m tyargs diff --git a/src/Compiler/FSharp.Compiler.Service.fsproj b/src/Compiler/FSharp.Compiler.Service.fsproj index e1fe24ea421..88398679c3f 100644 --- a/src/Compiler/FSharp.Compiler.Service.fsproj +++ b/src/Compiler/FSharp.Compiler.Service.fsproj @@ -21,6 +21,7 @@ $(OtherFlags) --warnon:3218 $(OtherFlags) --warnon:3390 + true $(IntermediateOutputPath)$(TargetFramework)\ $(IntermediateOutputPath)$(TargetFramework)\ @@ -76,8 +77,8 @@ - - + + @@ -365,8 +366,8 @@ - - + + @@ -504,12 +505,12 @@ - - - - - - + + + + + + diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ImportDeclarations/ImportDeclarations.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ImportDeclarations/ImportDeclarations.fs index 1f93d4b3bab..851746df35c 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ImportDeclarations/ImportDeclarations.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/ImportDeclarations/ImportDeclarations.fs @@ -107,4 +107,75 @@ module ImportDeclarations = |> verifyCompileAndRun |> shouldSucceed + [] + let ImplementImportedAbstractBaseMethodsFailsIfUsed ()= + FSharp """ +module Testing +open System.Text.Json +open System.Text.Json.Serialization + +type StringTrimJsonSerializer(o: JsonSerializerOptions) = + inherit JsonConverter() + + override this.Read(reader, _, _) = + match reader.TokenType with + | JsonTokenType.String -> reader.GetString().Trim() + | _ -> JsonException("Type is not a string") |> raise + + override this.Write(writer, objectToWrite, options) = base.Write(writer, objectToWrite, options) + +type SomeType = { AField: string } + +let serialize item = + let options = JsonSerializerOptions() + StringTrimJsonSerializer options |> options.Converters.Add + JsonSerializer.Serialize(item, options) + +[] +let main _ = + { AField = "a" } + |> serialize + |> ignore + 0""" + |> verifyCompile + |> shouldFail + |> withDiagnostics [ + (Error 1201, Line 15, Col 59, Line 15, Col 101, "Cannot call an abstract base member: 'Write'") + ] + + + [] + let ImplementImportedAbstractBaseMethodsFailsIfNotUsed ()= + FSharp """ +module Testing + +open System.Text.Json +open System.Text.Json.Serialization + +type StringTrimJsonSerializer(o: JsonSerializerOptions) = + inherit JsonConverter() + override this.Read(reader, _, _) = + match reader.TokenType with + | JsonTokenType.String -> reader.GetString().Trim() + | _ -> JsonException("Type is not a string") |> raise + override this.Write(writer, objectToWrite, options) = base.Write(writer, objectToWrite, options) + +type SomeType = { AField: int } + +let serialize item = + let options = JsonSerializerOptions() + StringTrimJsonSerializer options |> options.Converters.Add + JsonSerializer.Serialize(item, options) + +[] +let main _ = + { AField = 1 } + |> serialize + |>ignore + 0""" + |> verifyCompile + |> shouldFail + |> withDiagnostics [ + (Error 1201, Line 13, Col 59, Line 13, Col 101, "Cannot call an abstract base member: 'Write'") + ] diff --git a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/SuggestionsTests.fs b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/SuggestionsTests.fs index 50837863d5a..76bf2d3e88c 100644 --- a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/SuggestionsTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/SuggestionsTests.fs @@ -139,12 +139,12 @@ open Collectons [] let ``Suggest Namespaces`` () = FSharp """ -open System.Collectons +open System.Lema """ |> typecheck |> shouldFail - |> withSingleDiagnostic (Error 39, Line 2, Col 13, Line 2, Col 23, - "The namespace 'Collectons' is not defined.") + |> withSingleDiagnostic (Error 39, Line 2, Col 13, Line 2, Col 17, + "The namespace 'Lema' is not defined.") [] let ``Suggest Record Labels`` () = diff --git a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj index 38937398473..e1704555421 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj +++ b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj @@ -19,7 +19,7 @@ $(DefineConstants);DEBUG true - true + true @@ -228,9 +228,9 @@ - - - + + + @@ -280,7 +280,7 @@ - + @@ -313,14 +313,12 @@ - - %(RelativeDir)TestSource\%(Filename)%(Extension) - - - + + %(RelativeDir)TestSource\%(Filename)%(Extension) + %(RelativeDir)\BaseLine\%(Filename)%(Extension) @@ -331,6 +329,7 @@ + From 5a68434e46cf9dc7d277be423cba0e4166be6817 Mon Sep 17 00:00:00 2001 From: KevinRansom Date: Tue, 5 Mar 2024 00:44:34 -0800 Subject: [PATCH 2/5] fantomas --- src/Compiler/AbstractIL/il.fs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Compiler/AbstractIL/il.fs b/src/Compiler/AbstractIL/il.fs index 43ac3e930a9..24811cb62c0 100644 --- a/src/Compiler/AbstractIL/il.fs +++ b/src/Compiler/AbstractIL/il.fs @@ -5642,9 +5642,8 @@ let resolveILMethodRefWithRescope r (td: ILTypeDef) (mref: ILMethodRef) = possibles |> List.filter (fun md -> mref.CallingConv = md.CallingConv - && - (md.Parameters, argTypes) - ||> List.lengthsEqAndForall2 (fun p1 p2 -> r p1.Type = p2) + && (md.Parameters, argTypes) + ||> List.lengthsEqAndForall2 (fun p1 p2 -> r p1.Type = p2) && // REVIEW: this uses equality on ILType. For CMOD_OPTIONAL this is not going to be correct r md.Return.Type = retType) From 21587193d7b865e056cbefdd0af7497a24ef3be8 Mon Sep 17 00:00:00 2001 From: "Kevin Ransom (msft)" Date: Tue, 5 Mar 2024 02:13:39 -0800 Subject: [PATCH 3/5] Update src/Compiler/AbstractIL/il.fs Co-authored-by: Tomas Grosup --- src/Compiler/AbstractIL/il.fs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Compiler/AbstractIL/il.fs b/src/Compiler/AbstractIL/il.fs index 24811cb62c0..fecefad1434 100644 --- a/src/Compiler/AbstractIL/il.fs +++ b/src/Compiler/AbstractIL/il.fs @@ -1910,8 +1910,6 @@ type ILMethodDef metadataIndex: int32 ) = - let _x = () - new(name, attributes, implAttributes, callingConv, parameters, ret, body, isEntryPoint, genericParams, securityDecls, customAttrs) = ILMethodDef( name, From ee233b9fa918909d12f05a359ade84d6d43d17af Mon Sep 17 00:00:00 2001 From: "Kevin Ransom (msft)" Date: Tue, 5 Mar 2024 02:14:18 -0800 Subject: [PATCH 4/5] Update src/Compiler/FSharp.Compiler.Service.fsproj Co-authored-by: Tomas Grosup --- src/Compiler/FSharp.Compiler.Service.fsproj | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Compiler/FSharp.Compiler.Service.fsproj b/src/Compiler/FSharp.Compiler.Service.fsproj index 88398679c3f..5c11a4eebbf 100644 --- a/src/Compiler/FSharp.Compiler.Service.fsproj +++ b/src/Compiler/FSharp.Compiler.Service.fsproj @@ -21,7 +21,6 @@ $(OtherFlags) --warnon:3218 $(OtherFlags) --warnon:3390 - true $(IntermediateOutputPath)$(TargetFramework)\ $(IntermediateOutputPath)$(TargetFramework)\ From bd6296c0670baa457dae15e12b58604d7661345c Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 5 Mar 2024 12:56:25 +0100 Subject: [PATCH 5/5] Update 8.0.md --- docs/release-notes/.Language/8.0.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/release-notes/.Language/8.0.md b/docs/release-notes/.Language/8.0.md index f44c3df1f39..1e2dc00f2cd 100644 --- a/docs/release-notes/.Language/8.0.md +++ b/docs/release-notes/.Language/8.0.md @@ -1,7 +1,7 @@ ### Fixed -* Disallow using base to invoke an abstract base method. . ([Issue #13926](https://github.com/dotnet/fsharp/issues/13926), [PR #16773](https://github.com/dotnet/fsharp/pull/16773)) +* Disallow using base to invoke an abstract base method ([Issue #13926](https://github.com/dotnet/fsharp/issues/13926), [PR #16773](https://github.com/dotnet/fsharp/pull/16773)) ### Added -* `while!` ([Language suggestion #1038](https://github.com/fsharp/fslang-suggestions/issues/1038), [PR #14238](https://github.com/dotnet/fsharp/pull/14238)) \ No newline at end of file +* `while!` ([Language suggestion #1038](https://github.com/fsharp/fslang-suggestions/issues/1038), [PR #14238](https://github.com/dotnet/fsharp/pull/14238))