Fix [<RequireQualifiedAccess>] on a DU shadows types in the same module

[forki]

#1253 and #1294 show issues around name resolution. So let's fix these

• reproduce bug [<RequireQualifiedAccess>] on a DU shadows types in the same module #1253
• fix bug [<RequireQualifiedAccess>] on a DU shadows types in the same module #1253
• reproduce bug Type inference for a record/class behaves differently when module is open or not #1294
• fix bug Type inference for a record/class behaves differently when module is open or not #1294
• check that all other tests still work

[forki]

why are the other CIs not testing this?

[forki]

the proposed fix changes name resolution in the following way:

1. we still check if we find a union type
2. if the union type is fully-qualified then we report it back (showDeprecated = false)
3. then we check for tyCons
4. this is unrelated perf proposal that I can remove if needed: if the tyconSearch finds something then we report it back immediatly and don't look further
5. as always we check for module names
6. now comes the +++ and AtMostOneResult step - here we still keep order between tyconSearch and moduleSearch results. But we put the unionResults in the middle so that we still resolve the old union style with showDeprecated = true and issue the warning

[forki]

in order to fix #1294 as well I moved the unionSearch below the tyconSearch. This ensures that we always prefer types over union cases.

This change is a potential breaking change since we might actually break code that already compiled.

[dsyme]

@forki Awesome work. Will look closely soon. Will this be a breaking change in any situation? Thanks

[forki]

Tbh the breaking change is very very narrow. Basically the reverse of
#1294, but since this was a case that was different between using open and
using fully qualified name I would argue that it was incorrect that the
compiler actually compiled. I will try to come up with a sample to
illustrate this.

Am 02.09.2016 15:59 schrieb "Don Syme" notifications@github.com:

@forki https://github.com/forki Awesome work. Will look closely soon.
Will this be a breaking change in any situation? Thanks

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1512 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNJkjaAXfp1WKRSQiFMIZAadHBFy6ks5qmCukgaJpZM4JzqyY
.

[dsyme]

@OmarTawfik @forki @KevinRansom Anyone know why Jenkins CI is not running?

[forki]

I'm not sure anymore that this is breaking. 6b801ce suggests the case that I was thinking of still works.

[liboz]

It seems that this change means that given (basically removing the qualified access)

module A =
    type U = | C

    type C() =
        static member M() = ()

A.C will now prefer the constructor of the type C with the static member M() instead of the C from the discriminated union U. That could be a potentially breaking change

[forki]

@liboz

module A =
    type U = | C

    type C() =
        static member M() = ()

let x = A.C()

doesn't compile in current released version

error FS0003: This value is not a function and cannot be applied

So this case is fine.

[liboz]

@forki, try let x = A.C instead. I made a typo (which is now fixed!) in my first version.

[forki]

module A =
    type U = | C of unit

    type C() =
        static member M() = ()

let x : A.U = A.C()

currently compiles and would break afterwards. I guess that is the case I was looking for.
And yes the following is same:

module A =
    type U = | C

    type C() =
        static member M() = ()

let x : A.U = A.C

@dsyme so that's the risk...

@liboz: thanks for helping

[forki]

Another case that I broke:

module A = 
    type X = X

module B =
    type A.X with

        member this.Y() = 1

open B

let y  = A.X.Y()

But not sure yet why.

[forki]

ok seems my "perf optimization" did too much ;-) that last case is now fixed

[forki]

@dsyme @liboz: also

magic! These cases still work. So it looks this is not breaking!

[liboz]

Nice. I think though that #1294 is broken again though... I had been trying to work on these bugs as well, and got pretty stuck on how to fix #1294 and getting distracted by other stuff.

Maybe there's a way to make it so that the name resolution code is shared when you open a module and when you use the qualified name.

[matthid]

module A =
    type U = | C of unit

    type C() =
        static member M() = ()

let x : A.U = A.C()

feels like a bug IMHO

[forki]

@liboz: lol yes

OK the following observation:

If we fix #1294 like in 9d7ad99 then we break the cases that @liboz mentioned. Also the case that I reported in #1512 (comment) is a simplication of code from the F# compiler test sources (in a file called test.fsx). So we even break the compiler test cases ;-)

But after thinking about it I agree with @matthid. What the compiler did was wrong:

module A = 
    type X = X

module B =
    type A.X with

        member this.Y() = 1

open B

let x = A.X

let y  = A.X.Y()

In this last line A.X is actually a type and Y is a instance function. So correct thing would be:

let y = A.X.X.Y()

And sure enough after reverting ec68764 this works again.

[matthid]

In the "bug" situation can we even refer to the "C" with the "M" member (ie its constructor)?
I mean if the DU was defined after the class one could argue but the class is defined last...

[forki]

@matthid there is no order in this situation.

[forki]

ready for review

[OmarTawfik]

I see CI is running now. Please let me know if you see it happening again.

[dsyme]

@forki Could you merge with master? Thanks

[dsyme]

@forki Could you merge with master? Thanks

[forki]

done. sorry for delay

[forki]

looks good to go now.

[forki]

changed it to run the search only once

[forki]

all green

[dsyme]

Thanks, great to have this glitch fixed

[forki]

looks like it brougt new issues. #1830