Fix for issue 8351 #8354

smoothdeveloper · 2020-01-25T22:26:01Z

tentative fix for #8351, not sure of the implication, picking the most visible of both getter and setter when both are defined in AccessibilityLogic.fs, otherwise same logic as before when only either is defined.

smoothdeveloper · 2020-01-29T22:15:32Z

src/fsharp/AccessibilityLogic.fs

+            let getA = (resolveILMethodRef tdef mrefGet).Access
+            let setA = (resolveILMethodRef tdef mrefSet).Access
+            // pick most accessible
+            max getA setA


Please let me know if using max here is problematic and how you'd work around it.

cartermp · 2020-06-08T21:17:42Z

@KevinRansom @dsyme can you take a look?

…ssue as C#.

Added another property that is expected to fail, exhibiting inconsistencies in how the error is reported depending if the type is defined in F# or not.

KevinRansom · 2020-06-09T00:26:57Z

tentative fix for #8351, not sure of the implication, picking the most visible of both getter and setter when both are defined in AccessibilityLogic.fs, otherwise same logic as before when only either is defined.

Sigh!!! whilst this seems like a hack, if we look at how this function is used in the compiler, we can find this comment:

Here // For IL properties, we get an approximate accessibility that at least reports "internal" as "internal" and "private" as "private"

You have testing that shows, that later on we fix up the approximate nature of this resolution, and correctly fail when the setter is not visible here:(https://github.com/dotnet/fsharp/pull/8354/files#diff-ae28350224587d202ca4111c1125f62a)

Whilst I hate using the ordinal position of visibility in that list, it's probably the simplest and least error prone mechanism available. I do however reserve the right to moan about it continuously

cartermp · 2020-06-09T00:30:25Z

I do however reserve the right to moan about it continuously

KevinRansom · 2020-06-09T00:31:08Z

My role model ... right there

cartermp · 2020-06-10T17:05:31Z

Yay, tests pass! @KevinRansom further thoughts?

KevinRansom

Having a mechanism to determine visibility based on the "broadest visible item, is a good solution. I wish we didn't encode visibility broadness based on the ordinal position in the ILMemberAccess enum, however, this approach does accurately compute visibility.

Thanks for taking care of this

TIHan · 2020-06-12T00:38:20Z

src/absil/il.fsi

@@ -727,16 +727,17 @@ type ILMethodBody =
      SourceMarker: ILSourceMarker option }

 /// Member Access
+// important: order of definition matters for AccessibilityLogic.GetILAccessOfILPropInfo


We might want to avoid having to rely on the order of this.

@TIHan , I hate it too, however, every time I tried to reproduce the actual broadening algorithm, it ended up being horrific. One thing I think we could do, is to assign a specific value to each enum value, that way it is specific and deterministic, and not determined by the order in the source file.

But relying on the order of declarations is how we F#. 😄

@KevinRansom I think we can do this without having to do some broadening algorithm.

Lol you are not wrong ... but some developer who is not familiar with the ordering, might prefer to alphabetize the enum and break everything.
Arguably that is covered, by a comment that says, "the order is important".

Can we cover it with a test instead of a comment (or maybe choose a testing approach that makes comments actually useful by verifying their correctness with regard to the code)?

@jonsequitur I've just added such test in tests/fsharp/Compiler/ILMemberAccessTests.fs : https://github.com/dotnet/fsharp/pull/8354/files#diff-6415f4a32cd76160025147135e46baaa

If cases are added/removed, the test, if not updated, will fail, and if some of the existing comparisons don't hold true anymore, it will also fail.

Please let me know if that fits the bill.

Roslyn also does ordering: http://sourceroslyn.io/#Microsoft.CodeAnalysis/Symbols/Accessibility.cs , http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Symbols/PropertyOrEventSymbolExtensions.cs,b31562460f5b74cc,references

I think for us, we technically don't need it for correctness in the compiler because the compiler simply asks a question if it is visible or not - all we need to do is ask if the get or set method is visible. However, the tooling, such as Symbols.fs, does need to know which is more accessible in terms of ILMemberAccess.

If we are going to do ordering, let's make the order just like Roslyn:

[<RequireQualifiedAccess>] type ILMemberAccess = | CompilerControlled | Private | FamilyAndAssembly // protected and internal | Family // protected | Assembly // internal | FamilyOrAssembly // protected or internal | Public

@TIHan I've updated it to match your order, thanks!

TIHan · 2020-06-12T00:49:37Z

src/fsharp/AccessibilityLogic.fs

+    | FSProp (_, _, None, Some vref) -> IsValAccessible ad vref
+    | FSProp (_, _, Some vrefGet, Some vrefSet) -> 
+        // pick most accessible
+        IsValAccessible ad vrefGet || IsValAccessible ad vrefSet


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

@TIHan I think this is handled:

fsharp/src/fsharp/AccessibilityLogic.fs

Lines 255 to 265 in 1ccb4b1

| 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?

…h other

KevinRansom · 2020-06-13T07:53:36Z

@smoothdeveloper, i have an alternative way of doing the comparison that doesn't rely on the specification order of ILMemberAccess du.

I'm taking a crack at tests that use the compiler instead of ILMemberAccessTests, to verify the visibility calculation which is kind of a complicated way of verifying that ILMember orders don't change :-)

Anyway don't hate on me for this, if you can think of a better alternative or reasons why this won't worl I would welcome the feedback.

let GetILAccessOfILPropInfo (ILPropInfo(tinfo, pdef)) =
    let tdef = tinfo.RawMetadata
    let ilAccess =
        match pdef.GetMethod, pdef.SetMethod with 
        | Some mref, None 
        | None, Some mref -> (resolveILMethodRef tdef mref).Access

        | Some mrefGet, Some mrefSet ->
            let getA = (resolveILMethodRef tdef mrefGet).Access
            let setA = (resolveILMethodRef tdef mrefSet).Access

            // use the accessors to determine the visibility of the property
            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

        | None, None -> ILMemberAccess.Public

    ilAccess

smoothdeveloper · 2020-06-13T17:55:54Z

@KevinRansom,

The drawbacks I see:

changing the order of the match cases may break the behaviour
the order isn't defined in the DU itself (I assume we'd move the comment I've initially put there and minimize the diff by reverting to the shuffled order)
more boiler plate code in the compiler versus in a test
the invariant is now expressed only in implementation detail and can't turn the build red

Nonetheless, I'm ok to go with the consensus in the team about what is the best approach, just let me know for me to make the adjustments.

KevinRansom · 2020-06-14T01:06:11Z

@KevinRansom,

The drawbacks I see:

changing the order of the match cases may break the behaviour

the order isn't defined in the DU itself (I assume we'd move the comment I've initially put there and minimize the diff by reverting to the shuffled order)

more boiler plate code in the compiler versus in a test

the invariant is now expressed only in implementation detail and can't turn the build red

Nonetheless, I'm ok to go with the consensus in the team about what is the best approach, just let me know for me to make the adjustments.

@smoothdeveloper, I don't like it either and I'm wondering if a comparer might be a better. Honestly, it's annoying that the compiler needs a notion of property visibility rather than relying on the visibility of the method implementing the operation being performed. However, it is there, whats super fun, is this is only an approximation, because of the disconnect between F# visibilities and the CLR notion of visibilities.

All that said, at least the implementation detail this is dependent upon is the function responsible for doing this stuff.

Anyway, I'm going to look at it some more. I'm not confident that I love either solution yet.

KevinRansom · 2020-06-15T09:18:27Z

@smoothdeveloper please take a look at this PR: #9445 it contains additional testing. It no longer is dependent on DU ordering. But will perform a lot worse.

Co-authored-by: Gauthier Segay <[email protected]>

cartermp · 2020-06-23T17:48:50Z

@KevinRansom conflicts :)

smoothdeveloper changed the title ~~Fix for issue 8351~~ [wip] Fix for issue 8351 Jan 25, 2020

smoothdeveloper commented Jan 29, 2020

View reviewed changes

cartermp requested review from dsyme and TIHan February 7, 2020 21:35

smoothdeveloper mentioned this pull request Feb 15, 2020

add a failing test exhibiting issue #8351 #8352

Closed

smoothdeveloper added 6 commits June 8, 2020 14:23

add a failing test exhibiting issue dotnet#8351

7b958fd

updating VB and F# code to have private getter as well, VB has same i…

827e5b8

…ssue as C#.

Furthering the test, all languages exhibit the same issue.

b5e59d5

Added another property that is expected to fail, exhibiting inconsistencies in how the error is reported depending if the type is defined in F# or not.

F# setter on Prop1 works, despite the getter is internal

cccb3da

fix for 8351 (/!\ may break other things)

5986427

reshuffle ILMemberAccess so plain comparison works

b799113

smoothdeveloper force-pushed the fix-for-issue-8351 branch from 9b28332 to b799113 Compare June 8, 2020 21:24

smoothdeveloper changed the title ~~[wip] Fix for issue 8351~~ Fix for issue 8351 Jun 8, 2020

testConfig has a new version

8f94aff

KevinRansom approved these changes Jun 10, 2020

View reviewed changes

TIHan reviewed Jun 12, 2020

View reviewed changes

smoothdeveloper added 3 commits June 11, 2020 19:02

adding basic test around ILMemberAccess items being comparable to eac…

b5f5f6a

…h other

ceci n'est pas une fonction factorielle

1ccb4b1

codereview: redefine order of ILMemberAccess to match roslyn codebase

b024d51

Update for fix8351

b88ba3c

feedback

49d99be

smoothdeveloper mentioned this pull request Jun 17, 2020

General question on PR's, when several PR's have a dependency on a commone one #9480

Closed

KevinRansom and others added 3 commits June 20, 2020 00:42

Update src/fsharp/AccessibilityLogic.fs

fb3c52b

Co-authored-by: Gauthier Segay <[email protected]>

resolve conflicts

16deb23

merge

37be39e

KevinRansom mentioned this pull request Jun 22, 2020

Do not merge - alternate fix for issue 8351 kr #9445

Closed

Update

24b4d28

KevinRansom force-pushed the fix-for-issue-8351 branch from 61e4dfe to 24b4d28 Compare June 22, 2020 21:07

cartermp approved these changes Jun 22, 2020

View reviewed changes

TIHan approved these changes Jun 23, 2020

View reviewed changes

Merge branch 'master' into fix-for-issue-8351

8bfcdd6

KevinRansom merged commit 1ecc201 into dotnet:master Jun 23, 2020

abelbraaksma mentioned this pull request Jun 24, 2020

Mixed visibility of property getter/setter making property setter not usable in method / constructor invocation. #8351

Closed

smoothdeveloper mentioned this pull request May 5, 2021

Accessibility computations in tooling are not taking into account signature hiding, leading to glitches #11528

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue 8351 #8354

Fix for issue 8351 #8354

smoothdeveloper commented Jan 25, 2020

smoothdeveloper Jan 29, 2020

cartermp commented Jun 8, 2020

KevinRansom commented Jun 9, 2020

cartermp commented Jun 9, 2020

KevinRansom commented Jun 9, 2020

cartermp commented Jun 10, 2020

KevinRansom left a comment

TIHan Jun 12, 2020

KevinRansom Jun 12, 2020

jonsequitur Jun 12, 2020

TIHan Jun 12, 2020 •

edited

Loading

KevinRansom Jun 12, 2020

jonsequitur Jun 12, 2020

smoothdeveloper Jun 12, 2020

TIHan Jun 12, 2020

smoothdeveloper Jun 12, 2020

TIHan Jun 12, 2020

smoothdeveloper Jun 12, 2020

KevinRansom commented Jun 13, 2020

smoothdeveloper commented Jun 13, 2020

KevinRansom commented Jun 14, 2020

KevinRansom commented Jun 15, 2020

cartermp commented Jun 23, 2020

	\| 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

Fix for issue 8351 #8354

Fix for issue 8351 #8354

Conversation

smoothdeveloper commented Jan 25, 2020

Choose a reason for hiding this comment

cartermp commented Jun 8, 2020

KevinRansom commented Jun 9, 2020

cartermp commented Jun 9, 2020

KevinRansom commented Jun 9, 2020

cartermp commented Jun 10, 2020

KevinRansom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TIHan Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinRansom commented Jun 13, 2020

smoothdeveloper commented Jun 13, 2020

KevinRansom commented Jun 14, 2020

KevinRansom commented Jun 15, 2020

cartermp commented Jun 23, 2020

TIHan Jun 12, 2020 •

edited

Loading