Skip to content

Bugfix: Ref assemblies contained .property definitions at the wrong type in generated IL #14093

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
Oct 12, 2022
Merged

Bugfix: Ref assemblies contained .property definitions at the wrong type in generated IL #14093

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
7bd6d4b
Bugfix: Ref assemblies contained property definitions at the wrong type
T-Gro Oct 12, 2022
8d3d52b
Better comment
T-Gro Oct 12, 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
9 changes: 6 additions & 3 deletions src/Compiler/AbstractIL/ilwrite.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,7 +1192,7 @@ let canGenPropertyDef cenv (prop: ILPropertyDef) =
// If we have GetMethod or SetMethod set (i.e. not None), try and see if we have MethodDefs for them.
// NOTE: They can be not-None and missing MethodDefs if we skip generating them for reference assembly in the earlier pass.
// Only generate property if we have at least getter or setter, otherwise, we skip.
[| prop.GetMethod; prop.SetMethod |]
[| prop.GetMethod; prop.SetMethod |]
|> Array.choose id
|> Array.map (TryGetMethodRefAsMethodDefIdx cenv)
|> Array.exists (function | Ok _ -> true | _ -> false)
Expand Down Expand Up @@ -1304,11 +1304,14 @@ and GenTypeDefPass2 pidx enc cenv (tdef: ILTypeDef) =
// Now generate or assign index numbers for tables referenced by the maps.
// Don't yet generate contents of these tables - leave that to pass3, as
// code may need to embed these entries.
tdef.Implements |> List.iter (GenImplementsPass2 cenv env tidx)
props |> List.iter (GenPropertyDefPass2 cenv tidx)
tdef.Implements |> List.iter (GenImplementsPass2 cenv env tidx)
events |> List.iter (GenEventDefPass2 cenv tidx)
tdef.Fields.AsList() |> List.iter (GenFieldDefPass2 tdef cenv tidx)
tdef.Methods |> Seq.iter (GenMethodDefPass2 tdef cenv tidx)
// Generation of property definitions for **ref assemblies** is checking existence of generated method definitions.
// Therefore, due to mutable state within "cenv", order of operations matters.
// Who could have thought that using shared mutable state can bring unexpected bugs...?
props |> List.iter (GenPropertyDefPass2 cenv tidx)
tdef.NestedTypes.AsList() |> GenTypeDefsPass2 tidx (enc@[tdef.Name]) cenv
with exn ->
failwith ("Error in pass2 for type "+tdef.Name+", error: " + exn.Message)
Expand Down
127 changes: 118 additions & 9 deletions tests/fsharp/Compiler/CodeGen/EmittedIL/ReferenceAssemblyTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,115 @@ extends [runtime]System.Object

}"""]

[<Test>]
let ``Properties are emitted for CliMutable records`` () =
FSharp """
namespace ReferenceAssembly
type [<CLIMutable;NoComparison;NoEquality>] MyRecord = { MyId: int }"""
|> withOptions ["--refonly"]
|> compile
|> shouldSucceed
|> verifyIL [
referenceAssemblyAttributeExpectedIL
" .property instance int32 MyId()"]

[<Test>]
let ``Properties are emitted even for CliMutable records which are not last in a file`` () =
FSharp """
namespace ReferenceAssembly
type [<CLIMutable;NoComparison;NoEquality>] MyRecord = { MyId: int }
type [<CLIMutable;NoComparison;NoEquality>] MySecondRecord = { MySecondId: string }
"""
|> withOptions ["--refonly"]
|> compile
|> shouldSucceed
|> verifyIL [
referenceAssemblyAttributeExpectedIL
" .property instance int32 MyId()"
" .property instance string MySecondId()"]

[<Test>] // Regression https://github.com/dotnet/fsharp/issues/14088 .
// Generated IL was assigning properties to the last record in file instead of where they are supposed to be
let ``Properties are emitted for equal records in the same file`` () =
FSharp """
namespace Net7FSharpSnafu.Library

open System

type [<CLIMutable;NoComparison;NoEquality>] MyRecord =
{ Name: string }

type [<CLIMutable;NoComparison;NoEquality>] MySecondRecord = { Name: string }
"""
|> withOptions ["--refonly"]
|> compile
|> shouldSucceed
|> verifyIL [""" .class public auto ansi serializable sealed Net7FSharpSnafu.Library.MyRecord
extends [runtime]System.Object
{
.custom instance void [FSharp.Core]Microsoft.FSharp.Core.CLIMutableAttribute::.ctor() = ( 01 00 00 00 )
.custom instance void [FSharp.Core]Microsoft.FSharp.Core.NoComparisonAttribute::.ctor() = ( 01 00 00 00 )
.custom instance void [FSharp.Core]Microsoft.FSharp.Core.NoEqualityAttribute::.ctor() = ( 01 00 00 00 )
.custom instance void [FSharp.Core]Microsoft.FSharp.Core.CompilationMappingAttribute::.ctor(valuetype [FSharp.Core]Microsoft.FSharp.Core.SourceConstructFlags) = ( 01 00 02 00 00 00 00 00 )
.method public hidebysig specialname instance string
get_Name() cil managed
{
.custom instance void [runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )
.custom instance void [runtime]System.Diagnostics.DebuggerNonUserCodeAttribute::.ctor() = ( 01 00 00 00 )

.maxstack 8
IL_0000: ldnull
IL_0001: throw
}

.method public hidebysig specialname instance void
set_Name(string 'value') cil managed
{
.custom instance void [runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )
.custom instance void [runtime]System.Diagnostics.DebuggerNonUserCodeAttribute::.ctor() = ( 01 00 00 00 )

.maxstack 8
IL_0000: ldnull
IL_0001: throw
}

.method public specialname rtspecialname
instance void .ctor(string name) cil managed
{

.maxstack 8
IL_0000: ldnull
IL_0001: throw
}

.method public specialname rtspecialname
instance void .ctor() cil managed
{

.maxstack 8
IL_0000: ldnull
IL_0001: throw
}

.method public strict virtual instance string
ToString() cil managed
{
.custom instance void [runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )

.maxstack 8
IL_0000: ldnull
IL_0001: throw
}

.property instance string Name()
{
.custom instance void [FSharp.Core]Microsoft.FSharp.Core.CompilationMappingAttribute::.ctor(valuetype [FSharp.Core]Microsoft.FSharp.Core.SourceConstructFlags,
int32) = ( 01 00 04 00 00 00 00 00 00 00 00 00 )
.set instance void Net7FSharpSnafu.Library.MyRecord::set_Name(string)
.get instance string Net7FSharpSnafu.Library.MyRecord::get_Name()
}
} """]

[<Test>]
let ``Properties, getters, setters are emitted for internal properties`` () =
FSharp """
Expand Down Expand Up @@ -558,6 +667,11 @@ type MySecondaryAttribute() =
IL_0001: throw
}

.property instance int32 Prop1()
{
.set instance void ReferenceAssembly/MyAttribute::set_Prop1(int32)
.get instance int32 ReferenceAssembly/MyAttribute::get_Prop1()
}
}

.class auto ansi serializable nested public MySecondaryAttribute
Expand Down Expand Up @@ -597,11 +711,6 @@ type MySecondaryAttribute() =
IL_0001: throw
}

.property instance int32 Prop1()
{
.set instance void ReferenceAssembly/MyAttribute::set_Prop1(int32)
.get instance int32 ReferenceAssembly/MyAttribute::get_Prop1()
}
.property instance int32 Prop1()
{
.set instance void ReferenceAssembly/MySecondaryAttribute::set_Prop1(int32)
Expand Down Expand Up @@ -805,6 +914,10 @@ type Person(name : string, age : int) =
IL_0001: throw
}

.property instance bool Something()
{
.get instance bool ReferenceAssembly/CustomAttribute::get_Something()
}
}

.class auto ansi serializable nested public Person
Expand Down Expand Up @@ -865,10 +978,6 @@ type Person(name : string, age : int) =
IL_0001: throw
}

.property instance bool Something()
{
.get instance bool ReferenceAssembly/CustomAttribute::get_Something()
}
.property instance string Name()
{
.custom instance void ReferenceAssembly/CustomAttribute::.ctor(bool) = ( 01 00 01 00 00 )
Expand Down