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

There should be a warning for repeated variables even in curried arguments #6070

Closed
vicuna opened this issue Jul 9, 2013 · 11 comments
Closed

Comments

@vicuna
Copy link

vicuna commented Jul 9, 2013

Original bug ID: 6070
Reporter: kaustuv
Status: acknowledged (set by @damiendoligez on 2013-07-17T14:57:26Z)
Resolution: open
Priority: normal
Severity: feature
Version: 4.00.1
Category: typing
Monitored by: @gasche @hcarty

Bug description

I've just spent half an hour bug hunting an issue that came from code of this form (greatly simplified):

type error = E of string
let first_msg x (E x) = x

Obviously, this function is equivalent to fun _ (E x) -> x, which is silently the wrong interpretation of my intentions.

Basically, I would have spotted the error right away if instead I had written

let first_msg (x, E x) = x

but, since first_msg was being used in fold_left, I had to write it curried. The nature of this bug in my code took me on a complete wild goose chase as I hadn't assumed that the bug would be in my printing routine for errors.

Can there be a warning for shadowed variables in the arguments of curried function definitions? Note that this needs to be different from warning 27, which is way too annoying even to turn on, far less mark as error.

@vicuna
Copy link
Author

vicuna commented Jul 10, 2013

Comment author: @alainfrisch

Currently, "fun p1 p2 -> e" is treated exactly as "fun p1 -> fun p2 -> e". What would be the exact criterion to trigger the warning? Should it collect such immediately nested abstractions and check that they bind disjoint set of variables? What about the following:

let f x ?(a = x) x = a + x

Here we have a duplicated variable, but the instances are both used.

Personally, I find warning 27 quite useful (and it is turned into an error in LexiFi's code base). What's wrong with it?

@vicuna
Copy link
Author

vicuna commented Jul 10, 2013

Comment author: kaustuv

I think that if the user writes "fun x x -> x", then it is nearly always something strange and possibly unintentonal. This is also true for "fun x ?(a=x) x -> x".

Warning 27 also complains about such as "let snd x y = y" and if it is an error then I have to constantly fiddle with initial underscore. I am sure one can get used to it, but I would rather differentiate between "repeated uses of variable in function arguments" and "unused variable in function arguments" as potential sources of errors in my code.

@vicuna
Copy link
Author

vicuna commented Jul 17, 2013

Comment author: @damiendoligez

Maybe the criterion could be "the variable is shadowed before it is ever used", without regard for the shape of the expression. I don't much like a special warning just for function definitions.

@vicuna
Copy link
Author

vicuna commented Jul 17, 2013

Comment author: kaustuv

I think I am OK with that interpretation.

In fact, that could just be a new case of "suspicious unused variable" (warning 26). Although, enlarging the scope of existing warnings risks breaking existing code that mark those warnings as errors.

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 15, 2020
@garrigue
Copy link
Contributor

How could we implement @damiendoligez 's suggestion?
It would be natural to have warning 26 (enabled by default) in that case (shadowing of an unused variable). I.e., this is not innocuous.

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Aug 19, 2022
@shindere
Copy link
Contributor

shindere commented Aug 29, 2022 via email

@garrigue
Copy link
Contributor

I have no strong opinion on this one. There is already a warning for unused variables, which would apply in this case, so this could be a sufficient answer. @damiendoligez would be nicer for a warning enabled by default, but this is extra work for something that is already covered by another warning.

@github-actions github-actions bot removed the Stale label Aug 31, 2022
@shindere
Copy link
Contributor

shindere commented Sep 12, 2022 via email

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

4 participants