why are the other CIs not testing this?

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

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.

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

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
.

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

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

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

@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.

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

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

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.

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

@dsyme @liboz: also

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

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.

module A =
    type U = | C of unit

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

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

feels like a bug IMHO

@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.

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

@matthid there is no order in this situation.

ready for review

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

@forki Could you merge with master? Thanks

@forki Could you merge with master? Thanks

done. sorry for delay

looks good to go now.

changed it to run the search only once

all green

Thanks, great to have this glitch fixed

looks like it brougt new issues. #1830