Skip to content
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.

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

Fix for issue 8351 #8354

Merged
merged 17 commits into from
Jun 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
61 changes: 52 additions & 9 deletions src/fsharp/AccessibilityLogic.fs
Original file line number Diff line number Diff line change
Expand Up @@ -249,12 +249,53 @@ let private IsILMethInfoAccessible g amap m adType ad ilminfo =
let GetILAccessOfILPropInfo (ILPropInfo(tinfo, pdef)) =
let tdef = tinfo.RawMetadata
let ilAccess =
match pdef.GetMethod with
| Some mref -> (resolveILMethodRef tdef mref).Access
| None ->
match pdef.SetMethod with
| None -> ILMemberAccess.Public
| Some mref -> (resolveILMethodRef tdef mref).Access
match pdef.GetMethod, pdef.SetMethod with
| Some mref, None
| None, Some mref -> (resolveILMethodRef tdef mref).Access

| Some mrefGet, Some mrefSet ->
//
// Dotnet properties have a getter and a setter method, each of which can have a separate visibility public, protected, private etc ...
// This code computes the visibility for the property by choosing the most visible method. This approximation is usefull for cases
// where the compiler needs to know the visibility of the property.
// The specific ordering for choosing the most visible is:
// ILMemberAccess.Public,
// ILMemberAccess.FamilyOrAssembly
// ILMemberAccess.Assembly
// ILMemberAccess.Family
// ILMemberAccess.FamilyAndAssembly
// ILMemberAccess.Private
// ILMemberAccess.CompilerControlled
//
let getA = (resolveILMethodRef tdef mrefGet).Access
let setA = (resolveILMethodRef tdef mrefSet).Access

// Use the accessors to determine the visibility of the property.
// N.B. It is critical to keep the ordering in decreasing visibility order in the following match expression
match getA, setA with
| ILMemberAccess.Public, _
| _, ILMemberAccess.Public -> ILMemberAccess.Public

| ILMemberAccess.FamilyOrAssembly, _
| _, ILMemberAccess.FamilyOrAssembly -> ILMemberAccess.FamilyOrAssembly

| ILMemberAccess.Assembly, _
| _, ILMemberAccess.Assembly -> ILMemberAccess.Assembly

| ILMemberAccess.Family, _
| _, ILMemberAccess.Family -> ILMemberAccess.Family

| ILMemberAccess.FamilyAndAssembly, _
| _, ILMemberAccess.FamilyAndAssembly -> ILMemberAccess.FamilyAndAssembly

| ILMemberAccess.Private, _
| _, ILMemberAccess.Private -> ILMemberAccess.Private

| ILMemberAccess.CompilerControlled, _
| _, ILMemberAccess.CompilerControlled -> ILMemberAccess.CompilerControlled

| None, None -> ILMemberAccess.Public

ilAccess

let IsILPropInfoAccessible g amap m ad pinfo =
Expand Down Expand Up @@ -323,8 +364,11 @@ let IsMethInfoAccessible amap m ad minfo = IsTypeAndMethInfoAccessible amap m ad

let IsPropInfoAccessible g amap m ad = function
| ILProp ilpinfo -> IsILPropInfoAccessible g amap m ad ilpinfo
| FSProp (_, _, Some vref, _)
| FSProp (_, _, _, Some vref) -> IsValAccessible ad vref
| FSProp (_, _, Some vref, None)
| FSProp (_, _, None, Some vref) -> IsValAccessible ad vref
| FSProp (_, _, Some vrefGet, Some vrefSet) ->
// pick most accessible
IsValAccessible ad vrefGet || IsValAccessible ad vrefSet
Copy link
Contributor

Choose a reason for hiding this comment

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

So this makes a lot of sense. I feel like we need to do something similar on the ILProp side, maybe inside IsILPropInfoAccessible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TIHan I think this is handled:

| Some mrefGet, Some mrefSet ->
let getA = (resolveILMethodRef tdef mrefGet).Access
let setA = (resolveILMethodRef tdef mrefSet).Access
// pick most accessible
max getA setA
| None, None -> ILMemberAccess.Public
ilAccess
let IsILPropInfoAccessible g amap m ad pinfo =
let ilAccess = GetILAccessOfILPropInfo pinfo
IsILTypeAndMemberAccessible g amap m ad ad pinfo.ILTypeInfo ilAccess

it returns the max accessible in GetILAccessOfILPropInfo which is then checked against IsILTypeAndMemberAccessible.

Does that make sense?

#if !NO_EXTENSIONTYPING
| ProvidedProp (amap, tppi, m) as pp->
let access =
Expand All @@ -343,4 +387,3 @@ let IsPropInfoAccessible g amap m ad = function

let IsFieldInfoAccessible ad (rfref:RecdFieldInfo) =
IsAccessible ad rfref.RecdField.Accessibility

19 changes: 15 additions & 4 deletions tests/FSharp.Test.Utilities/TestFramework.fs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ module Commands =
let csc exec cscExe flags srcFiles =
exec cscExe (sprintf "%s %s" flags (srcFiles |> Seq.ofList |> String.concat " "))

let vbc exec vbcExe flags srcFiles =
exec vbcExe (sprintf "%s %s" flags (srcFiles |> Seq.ofList |> String.concat " "))

let fsi exec fsiExe flags sources =
exec fsiExe (sprintf "%s %s" flags (sources |> Seq.ofList |> String.concat " "))

Expand Down Expand Up @@ -123,6 +126,8 @@ type TestConfig =
{ EnvironmentVariables : Map<string, string>
CSC : string
csc_flags : string
VBC : string
vbc_flags : string
BUILD_CONFIG : string
FSC : string
fsc_flags : string
Expand Down Expand Up @@ -213,7 +218,8 @@ let config configurationName envVars =
let artifactsPath = repoRoot ++ "artifacts"
let artifactsBinPath = artifactsPath ++ "bin"
let coreClrRuntimePackageVersion = "3.0.0-preview-27318-01"
let csc_flags = "/nologo"
let csc_flags = "/nologo"
let vbc_flags = "/nologo"
let fsc_flags = "-r:System.Core.dll --nowarn:20 --define:COMPILED"
let fsi_flags = "-r:System.Core.dll --nowarn:20 --define:INTERACTIVE --maxerrors:1 --abortonerror"
let operatingSystem = getOperatingSystem ()
Expand All @@ -223,6 +229,7 @@ let config configurationName envVars =
let requirePackage = requireFile packagesDir
let requireArtifact = requireFile artifactsBinPath
let CSC = requirePackage ("Microsoft.Net.Compilers" ++ "2.7.0" ++ "tools" ++ "csc.exe")
let VBC = requirePackage ("Microsoft.Net.Compilers" ++ "2.7.0" ++ "tools" ++ "vbc.exe")
let ILDASM_EXE = if operatingSystem = "win" then "ildasm.exe" else "ildasm"
let ILDASM = requirePackage (("runtime." + operatingSystem + "-" + architectureMoniker + ".Microsoft.NETCore.ILDAsm") ++ coreClrRuntimePackageVersion ++ "runtimes" ++ (operatingSystem + "-" + architectureMoniker) ++ "native" ++ ILDASM_EXE)
let ILASM_EXE = if operatingSystem = "win" then "ilasm.exe" else "ilasm"
Expand All @@ -231,6 +238,7 @@ let config configurationName envVars =
let coreclrdll = requirePackage (("runtime." + operatingSystem + "-" + architectureMoniker + ".Microsoft.NETCore.Runtime.CoreCLR") ++ coreClrRuntimePackageVersion ++ "runtimes" ++ (operatingSystem + "-" + architectureMoniker) ++ "native" ++ CORECLR_DLL)
let PEVERIFY_EXE = if operatingSystem = "win" then "PEVerify.exe" else "PEVerify"
let PEVERIFY = requireArtifact ("PEVerify" ++ configurationName ++ peverifyArchitecture ++ PEVERIFY_EXE)
// let FSI_FOR_SCRIPTS = artifactsBinPath ++ "fsi" ++ configurationName ++ fsiArchitecture ++ "fsi.exe"
let FSharpBuild = requireArtifact ("FSharp.Build" ++ configurationName ++ fsharpBuildArchitecture ++ "FSharp.Build.dll")
let FSharpCompilerInteractiveSettings = requireArtifact ("FSharp.Compiler.Interactive.Settings" ++ configurationName ++ fsharpCompilerInteractiveSettingsArchitecture ++ "FSharp.Compiler.Interactive.Settings.dll")

Expand Down Expand Up @@ -266,7 +274,8 @@ let config configurationName envVars =
ILDASM = ILDASM
ILASM = ILASM
PEVERIFY = PEVERIFY
CSC = CSC
VBC = VBC
CSC = CSC
BUILD_CONFIG = configurationName
FSC = FSC
FSI = FSI
Expand All @@ -277,9 +286,10 @@ let config configurationName envVars =
FSharpBuild = FSharpBuild
FSharpCompilerInteractiveSettings = FSharpCompilerInteractiveSettings
csc_flags = csc_flags
fsc_flags = fsc_flags
fsc_flags = fsc_flags
fsi_flags = fsi_flags
Directory=""
vbc_flags = vbc_flags
Directory=""
DotNetExe = dotNetExe
DefaultPlatform = defaultPlatform }

Expand Down Expand Up @@ -506,6 +516,7 @@ let fscBothToOut cfg out arg = Printf.ksprintf (Commands.fsc cfg.Directory (exec
let fscBothToOutExpectFail cfg out arg = Printf.ksprintf (Commands.fsc cfg.Directory (execBothToOutExpectFail cfg cfg.Directory out) cfg.DotNetExe cfg.FSC) arg
let fscAppendErrExpectFail cfg errPath arg = Printf.ksprintf (Commands.fsc cfg.Directory (execAppendErrExpectFail cfg errPath) cfg.DotNetExe cfg.FSC) arg
let csc cfg arg = Printf.ksprintf (Commands.csc (exec cfg) cfg.CSC) arg
let vbc cfg arg = Printf.ksprintf (Commands.vbc (exec cfg) cfg.VBC) arg
let ildasm cfg arg = Printf.ksprintf (Commands.ildasm (exec cfg) cfg.ILDASM) arg
let ilasm cfg arg = Printf.ksprintf (Commands.ilasm (exec cfg) cfg.ILASM) arg
let peverify cfg = Commands.peverify (exec cfg) cfg.PEVERIFY "/nologo"
Expand Down
224 changes: 224 additions & 0 deletions tests/fsharp/Compiler/Conformance/Properties/ILMemberAccessTests.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information.

namespace FSharp.Compiler.UnitTests

open FSharp.Compiler.SourceCodeServices
open FSharp.Reflection
open FSharp.Test
open FSharp.Test.Utilities
open FSharp.Test.Utilities.Utilities
open NUnit.Framework

[<TestFixture>]
module ILMemberAccessTests =

let csharpBaseClass = """
namespace ExhaustiveCombinations
{
public class CSharpBaseClass
{
public string GetPublicSetInternal { get; internal set; }
public string GetPublicSetProtected { get; protected set; }
public string GetPublicSetPrivateProtected { get; private protected set; }
public string GetPublicSetProtectedInternal { get; protected internal set; }
public string GetPublicSetPrivate { get; private set; }

public string SetPublicGetInternal { internal get; set; }
public string SetPublicGetProtected { protected get; set; }
public string SetPublicGetPrivateProtected { private protected get; set; }
public string SetPublicGetProtectedInternal { protected internal get; set; }
public string SetPublicGetPrivate { private get; set; }
}
}
"""

let fsharpBaseClass = """
namespace ExhaustiveCombinations

open System

type FSharpBaseClass () =

member this.GetPublicSetInternal with public get() = "" and internal set (parameter:string) = ignore parameter
member this.GetPublicSetPrivate with public get() = "" and private set (parameter:string) = ignore parameter
member this.SetPublicGetInternal with internal get() = "" and public set (parameter:string) = ignore parameter
member this.SetPublicGetPrivate with private get() = "" and public set (parameter:string) = ignore parameter

"""


[<Test>]
let ``VerifyVisibility of Properties C# class F# derived class -- AccessPublicStuff`` () =

let fsharpSource =
fsharpBaseClass + """
open System
open ExhaustiveCombinations

type MyFSharpClass () =
inherit CSharpBaseClass()

member this.AccessPublicStuff() =

this.GetPublicSetInternal <- "1" // Inaccessible
let _ = this.GetPublicSetInternal // Accessible

this.GetPublicSetPrivateProtected <- "1" // Accessible
let _ = this.GetPublicSetPrivateProtected // Accessible

this.GetPublicSetProtectedInternal <- "1" // Accessible
let _ = this.GetPublicSetProtectedInternal // Accessible

this.GetPublicSetProtected <- "1" // Accessible
let _ = this.GetPublicSetProtected // Accessible

this.GetPublicSetPrivate <- "1" // Inaccessible
let _ = this.GetPublicSetPrivate // Accessible
()
"""

let csCmpl =
CompilationUtil.CreateCSharpCompilation(csharpBaseClass, CSharpLanguageVersion.CSharp8, TargetFramework.NetCoreApp30)
|> CompilationReference.Create

let fsCmpl =
Compilation.Create(fsharpSource, Fsx, Exe, options = [|"--langversion:preview"|], cmplRefs = [csCmpl])

CompilerAssert.CompileWithErrors(fsCmpl, [|
(FSharpErrorSeverity.Error, 491, (22, 9, 22, 41),
"The member or object constructor 'GetPublicSetInternal' is not accessible. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.");
(FSharpErrorSeverity.Error, 491, (25, 9, 25, 49),
"The member or object constructor 'GetPublicSetPrivateProtected' is not accessible. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.");
(FSharpErrorSeverity.Error, 491, (34, 9, 34, 40),
"The member or object constructor 'GetPublicSetPrivate' is not accessible. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.")|])


[<Test>]
let ``VerifyVisibility of Properties C# class F# non-derived class -- AccessPublicStuff`` () =

let fsharpSource =
fsharpBaseClass + """
open System
open ExhaustiveCombinations

type MyFSharpClass () =

member _.AccessPublicStuff() =
let bc = new CSharpBaseClass()

bc.GetPublicSetInternal <- "1" // Inaccessible
let _ = bc.GetPublicSetInternal // Accessible

bc.GetPublicSetPrivateProtected <- "1" // Inaccessible
let _ = bc.GetPublicSetPrivateProtected // Accessible

bc.GetPublicSetProtectedInternal <- "1" // Accessible
let _ = bc.GetPublicSetProtectedInternal // Inaccessible

bc.GetPublicSetProtected <- "1" // Inaccessible
let _ = bc.SetPublicGetProtected // Accessible

bc.GetPublicSetPrivate <- "1" // Inaccessible
let _ = bc.GetPublicSetPrivate // Accessible
()
"""

let csCmpl =
CompilationUtil.CreateCSharpCompilation(csharpBaseClass, CSharpLanguageVersion.CSharp8, TargetFramework.NetCoreApp30)
|> CompilationReference.Create

let fsCmpl =
Compilation.Create(fsharpSource, Fsx, Exe, options = [|"--langversion:preview"|], cmplRefs = [csCmpl])

CompilerAssert.CompileWithErrors(fsCmpl, [|
(FSharpErrorSeverity.Error, 491, (22, 9, 22, 39),
"The member or object constructor 'GetPublicSetInternal' is not accessible. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.");
(FSharpErrorSeverity.Error, 491, (25, 9, 25, 47),
"The member or object constructor 'GetPublicSetPrivateProtected' is not accessible. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.");
(FSharpErrorSeverity.Error, 491, (28, 9, 28, 48),
"The member or object constructor 'GetPublicSetProtectedInternal' is not accessible. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.");
(FSharpErrorSeverity.Error, 491, (31, 9, 31, 40),
"The member or object constructor 'GetPublicSetProtected' is not accessible. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.");
(FSharpErrorSeverity.Error, 491, (32, 17, 32, 41),
"The member or object constructor 'SetPublicGetProtected' is not accessible. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.");
(FSharpErrorSeverity.Error, 491, (34, 9, 34, 38),
"The member or object constructor 'GetPublicSetPrivate' is not accessible. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions.")|])


[<Test>]
let ``VerifyVisibility of Properties F# base F# derived class -- AccessPublicStuff`` () =

let fsharpSource =
fsharpBaseClass + """
open System
open ExhaustiveCombinations

type MyFSharpClass () =
inherit FSharpBaseClass()

member this.AccessPublicStuff() =

this.GetPublicSetInternal <- "1" // Inaccessible
let _ = this.GetPublicSetInternal // Accessible

this.GetPublicSetPrivate <- "1" // Inaccessible
let _ = this.GetPublicSetPrivate // Accessible

this.SetPublicGetInternal <- "1" // Accessible
let _ = this.SetPublicGetInternal // Inaccessible

this.SetPublicGetPrivate <- "1" // Accessible
let _ = this.SetPublicGetPrivate // accessible

()
"""

let csCmpl =
CompilationUtil.CreateCSharpCompilation(csharpBaseClass, CSharpLanguageVersion.CSharp8, TargetFramework.NetCoreApp30)
|> CompilationReference.Create

let fsCmpl =
Compilation.Create(fsharpSource, Fsx, Exe, options = [|"--langversion:preview"|], cmplRefs = [csCmpl])

CompilerAssert.CompileWithErrors(fsCmpl, [|
(FSharpErrorSeverity.Error, 810, (25, 9, 25, 33),
"Property 'GetPublicSetPrivate' cannot be set");
(FSharpErrorSeverity.Error, 807, (32, 17, 32, 41),
"Property 'SetPublicGetPrivate' is not readable")
|])


[<Test>]
let ``VerifyVisibility of Properties F# class F# non-derived class -- AccessPublicStuff`` () =

let fsharpSource =
fsharpBaseClass + """
open System
open ExhaustiveCombinations

type MyFSharpClass () =

member _.AccessPublicStuff() =
let bc = new FSharpBaseClass()

bc.GetPublicSetInternal <- "1" // Inaccessible
let _ = bc.GetPublicSetInternal // Accessible

bc.GetPublicSetPrivate <- "1" // Inaccessible
let _ = bc.GetPublicSetPrivate // Accessible
()
"""

let csCmpl =
CompilationUtil.CreateCSharpCompilation(csharpBaseClass, CSharpLanguageVersion.CSharp8, TargetFramework.NetCoreApp30)
|> CompilationReference.Create

let fsCmpl =
Compilation.Create(fsharpSource, Fsx, Exe, options = [|"--langversion:preview"|], cmplRefs = [csCmpl])

CompilerAssert.CompileWithErrors(fsCmpl, [|
(FSharpErrorSeverity.Error, 810, (25, 9, 25, 31),
"Property 'GetPublicSetPrivate' cannot be set")|])



1 change: 1 addition & 0 deletions tests/fsharp/FSharpSuite.Tests.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
<Compile Include="Compiler\CodeGen\EmittedIL\CeEdiThrow.fs" />
<Compile Include="Compiler\Conformance\DataExpressions\ComputationExpressions.fs" />
<Compile Include="Compiler\Conformance\BasicGrammarElements\BasicConstants.fs" />
<Compile Include="Compiler\Conformance\Properties\ILMemberAccessTests.fs" />
<Compile Include="Compiler\ConstraintSolver\PrimitiveConstraints.fs" />
<Compile Include="Compiler\ConstraintSolver\MemberConstraints.fs" />
<Compile Include="Compiler\Warnings\AssignmentWarningTests.fs" />
Expand Down
Loading