[<RequireQualifiedAccess>] allows access to DU member if qualified only by module name

[KevinRansom]

This was originally opened at codepelex by latkin

Repro in VS 2013. Reported by Tomas P via fsbugs.

The RequireQualifiedAccess attribute should force users to include the DU name when accessing a DU member. However as shown below, it incorrectly allows access using only the module name.

module First = 
  [<RequireQualifiedAccess>]
  type DU = Member of int

module Second = 
  open First
  let a = Member(0) // Error (as expected)
  let b = DU.Member(0) // Ok (as expected)
  let c = First.Member(0) // This works too! (unexpected!)

Per Don:
This is a bug – the code below should give an error. We need to fix this in F# vNext.

It is possible we will need to make it a “this has been deprecated, please adjust your code” warning instead to reduce the impact of the fix.

[forki]

I think I found the issue. In https://github.com/forki/visualfsharp/blob/fsharp4/src/fsharp/nameres.fs#L1653 we search for UnionCases inside of modules. But we should only do this for Unions without RequireQualifiedAccessAttribute.

As a quickhack I checked if the DU in question has any attributes:
forki@3d3c5a9#diff-3dc00fcce72defacd0599c11cc2fb041R1654

This solves all DU access cases for me, but of course it's not the correct fix. What I really want is to check if we have RequiredQualifiedAttribute inside of tyCon.Attribs. But here I'm stuck.

I already discoverd things like

TyconRefHasAttribute g m g.attrib_ExtensionAttribute tcrefOfStaticClass

but I don't have a TcGlobals at that point, right?

If someone can point me in the right direction then I could try to come up with a PR which contains tests and a better fix.

[dsyme]

There's a TcGlobals in ncenv.

Here are the notes from our discussion on twitter:

• For [<RequireQualifiedAccess>] allows access to DU member if qualified only by module name #95.... Check attribute with HasFSharpAttribute g g.attrib_RequireQualifiedAccessAttribute tcref.Attribs.
• I think you will want to report the "this is deprecated" warning at the point where the Item.UnionCase is consumed in tc.fs
• To do this, add a boolean field to Item.UnionCase indicating the resolution needs a deprecation warning.
• Populate the flag in ResolvePatternLongIdentInModuleOrNamespace and ResolveExprLongIdentInModuleOrNamespace (or TryFindTypeWithUnionCase)
• Strictly speaking I guess you could report the warning directly in nameres.fs - I don't think any harm would come from it. But t's not really "normal" to report warnings from nameres.fs because some of the name resolution code does get reused by intellisense etc. So stick to the pattern and produce warnings in tc.fs instead.
• BTW I think ResolveFieldInModuleOrNamespace has the same problem, e.g. "Module.field1" is not checking for RequireQualifiedAccessAttribute
• For the deprecation warning message, use something like "This construct is deprecated in F# 4.0 and an error may be given when using this construct in later versions. The union type for union case 'XXX' has the RequireQualifiedAccessAttribute. Include the name of the union type in the name you are using." Or something like that

[forki]

ha, I'm making progress ;-)

[forki]

@tpetricek could you please review?

[forki]

as @dsyme already pointed out we have the same issue for records.

module M1 =
    [<RequireQualifiedAccess>]
    type R1 = { Field1 : int }

open M1
let r1 = { R1.Field1 = 42 } // correct
let r2 = { M1.Field1 = 42 } // compiles, but shoud not
let r3 = { Field1 = 42 }    // doesn't compile - as expected

I'll try to update #103 with a fix for this.

[forki]

ok the record case seems much harder to fix since we don't return Item.RecdField in https://github.com/Microsoft/visualfsharp/blob/5eb0657f6d9a5e4399062325a627c6c1812174cb/src/fsharp/nameres.fs#L2145

ideas?

[forki]

It's possibe to raise the warning directly in https://github.com/Microsoft/visualfsharp/blob/5eb0657f6d9a5e4399062325a627c6c1812174cb/src/fsharp/nameres.fs#L2145 (this is probably less clean as @dsyme described above, but also less invasive).

Strangely the code is processed twice. It seems it needs further investigation.

[forki]

Ok the message is raised twice sind modulSearch is called twice in https://github.com/Microsoft/visualfsharp/blob/5eb0657f6d9a5e4399062325a627c6c1812174cb/src/fsharp/nameres.fs#L2208

[dsyme]

It looks like you'd need to return a pair (RecdFieldRef * bool) . Add a new type FieldResolution = FieldResolution of RecdFieldRef * bool to nameres.fs/fsi for this pair. Then flow the extra info through to ResolveField and into BuildFieldMap.

Some quite fiddly work in BuildRefMap is then needed, until you actually get the flag through to the point where we commit to a partifular resolution for the field at the end of that function, at the point where we call "CheckRecdFieldAccessible".

FWIW BuildFieldMap is intersecting the possible types being constructed so "{A=e1;B=e2}" and "{A=e1;C=e2}" may resolve to different record type constructions if {A,B} and {A,C} are enough to disambiguate which record type is being constructed.

[huwman]

Hi, is this issue resolved in the version of F# available in Visual Studio 2015 rc?

[latkin]

@huwman yes, it should be