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

Doubtful "unused open" warning on module used for record field disambiguation #7443

Closed
vicuna opened this issue Dec 28, 2016 · 5 comments
Closed
Assignees

Comments

@vicuna
Copy link

vicuna commented Dec 28, 2016

Original bug ID: 7443
Reporter: @gasche
Assigned to: @Octachron
Status: resolved (set by @gasche on 2016-12-29T19:08:47Z)
Resolution: fixed
Priority: normal
Severity: tweak
Version: 4.05.0 +dev/beta1/beta2/beta3/rc1
Fixed in version: 4.05.0 +dev/beta1/beta2/beta3/rc1
Category: typing
Related to: #7386
Monitored by: @rixed

Bug description

The following code fails to compile with "-w @A" (setting all warnings as errors):

module M = struct
type t = { x : int; y : int }
end

let sum : M.t -> int = function
| M.{ x; y } -> x + y

The compiler complains that the opening "M." in the pattern M.{ x; y } is unused. But if I remove "M.", with the warning-as-errors setting, I still get a compilation error!

I believe that access to a module record field for disambiguation purposes should be counted as a "use" of this module. One might argue that this needs not be the case if neither warnings 40 or 42 are set, but I would personally still count it as a "use" in those cases -- I want to be able to write code that respects the 40-as-an-error principle even if 40 is not an error or even not set.

Additional information

This issue can easily be worked around by writing the pattern { M.x; y } instead, but I personally find M.{ x; y } more elegant and, beside, the warning behavior may surprise innocent bystanders -- hence it is arguably a usability bug.

@vicuna
Copy link
Author

vicuna commented Dec 28, 2016

Comment author: @gasche

I just noticed (in real code) that this also happens with variant constructor disambiguation:

module M = struct
type t = A | B | C
end

let sum : M.t -> unit = function
| M.(A | B | C) -> ()

Again the compiler complains that "M." is unused. This is again wrong and, furthermore, there is no workaround as in the record case -- at least as long as 7386 is not resolved.

@vicuna
Copy link
Author

vicuna commented Dec 28, 2016

Comment author: @Octachron

I fear that this nothing but a simple mistake on my side when introducing pattern open: see the short fix in #990 .

@vicuna
Copy link
Author

vicuna commented Dec 29, 2016

Comment author: @gasche

I can confirm that the fix by octachron in #990 fixed the problem -- I tested those two examples.

I should have realized that this was not an issue with "open" in general, but with the new pattern-open feature, as "let open M in function A | B | C -> ()" does not warn -- I failed to try.

Thanks octachron for the quick fix and the nice feature.

@vicuna vicuna closed this as completed Dec 29, 2016
@vicuna
Copy link
Author

vicuna commented May 8, 2017

Comment author: @lpw25

This fix doesn't look correct to me. It looks as though it just turns off the unused open warning for opens in patterns, which is not the desired behaviour.

@vicuna
Copy link
Author

vicuna commented May 8, 2017

Comment author: @lpw25

Actually, nevermind. I see that the change is to turn off the warning during the contains_gadt check. Sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants