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

no warning when ignoring a partially applied function in a match with expression #6881

Closed
vicuna opened this issue May 28, 2015 · 12 comments
Closed

Comments

@vicuna
Copy link

vicuna commented May 28, 2015

Original bug ID: 6881
Reporter: domsj
Status: acknowledged (set by @damiendoligez on 2015-06-01T20:05:47Z)
Resolution: open
Priority: normal
Severity: feature
Version: 4.02.1
Category: typing
Monitored by: @gasche

Bug description

The following code does not emit any warnings

let () =
  match Hashtbl.find with
  | exception Not_found -> ()
  | _ ->
    (* do stuff ... *)
    ()

While this does:

let () =
  match Hashtbl.find with
  | exception Not_found -> ()
  | a ->
    ignore a;
    (* do stuff ... *)
    ()

I would've expected the same warning in both cases.

@vicuna
Copy link
Author

vicuna commented May 28, 2015

Comment author: @yallop

The warning is coming from the line:

ignore a

since 'a' is a function and you're using it without passing any arguments. Since that expression doesn't appear in the first piece of code, there's no need for a warning.

@vicuna
Copy link
Author

vicuna commented May 28, 2015

Comment author: @yallop

To be a little more explicit, this has nothing to do with 'match .. with'. You'll see the same warning with

let a = Hashtbl.find in ignore a

@vicuna
Copy link
Author

vicuna commented May 28, 2015

Comment author: domsj

I know where the warning comes from.
It's the absence of the warning in the first case which bit me...

@vicuna
Copy link
Author

vicuna commented May 28, 2015

Comment author: @yallop

The best way to avoid the problem is to avoid discarding results, e.g. by using Hashtbl.mem instead:

if Hashtbl.mem t k then (* do stuff *)

@vicuna
Copy link
Author

vicuna commented May 28, 2015

Comment author: @gasche

I think it would be hard to warn in the general case, because right now the idiomatic way to turn off the warning on (ignore <...>) when <...> has a function type (but you still want to force its evaluation) is to write (let _ = <...> in) instead. I would assume a non-neglectible amount of code has been written in this style and warning on the pattern _ is thus counter to the programmers intention.

(Note that performing a full eta-expansion (ignore (fun x y -> <...> x y)) would be another work-around, which only fail on rectypes function taking infinitely many arguments...)

If we warned only in the case of (match ... with exception ... | _ -> ...), how could an user turn off the warning when the expression to be forced is of function type? The problem is that (match ignore (...) with exception ... | () -> ...) does not work.

@vicuna
Copy link
Author

vicuna commented May 28, 2015

Comment author: domsj

Ok, I'll admit it's a bad example.

It's not what I was actually doing, I just wanted to create a small example here.

In my actual code I called a function to compute some result. Instead of returning an option when no result could be computed instead it throws a specific exception. (Changed that now to return an option though, to avoid further mistakes.)

At a certain point I just wanted to know whether it could compute a result or not. Being lazy I wrote some code like in the first example, and forgot to apply some parameters.
Luckily the test suite helped me out, but I would've preferred the compiler to warn me about this...

@vicuna
Copy link
Author

vicuna commented May 28, 2015

Comment author: @gasche

Jeremy, I find your response is only half-convincing. If there was always a better way to write code than

match $scrut with
| exception $handlers
| _ -> $body

then precisely it would make sense to emit a warning saying "writing this is always a mistake, please use ... instead". However, it can only be rewritten into

(try $scrut with $handlers); $body

when the handlers (and $body) return unit, but it is not possible in general. This is a general (and convenient) pattern for "evaluate this expression for its side-effect only, and let me return different results depending on its success or exception". It can be rewritten as Hashtbl.mem in the precise case of Hashtbl.find, but that does not mean other users won't encounter the problem on similar functions.

I think one solution would be to consistently warn if $scrut is not of type unit, recommending to users to write

match let _ = $scrut in () with
| exception $handlers
| _ -> $body

instead, but it seems a bit far-fetched.

@vicuna
Copy link
Author

vicuna commented Jun 1, 2015

Comment author: @damiendoligez

Note that performing a full eta-expansion (ignore (fun x y -> <...> x y)) would be another work-around

FTR, that doesn't work at all.

@vicuna
Copy link
Author

vicuna commented Jun 1, 2015

Comment author: @yallop

It can be rewritten as Hashtbl.mem in the precise case of Hashtbl.find, but that does not mean other users won't encounter the problem on similar functions.

That's a good point. One (slightly heavy) workaround is to annotate the wildcard pattern with the result type

match $scrut with
| exception $handlers
| (_ : t) -> $body

I suppose it'd be possible to warn for a non-annotated wildcard pattern where it's the only value pattern in a 'let' or a 'match'. In other words, this is fine, since the T pattern restricts the type:

match e with
T -> "T"
| _ -> "not T"

but this could trigger a warning

match e with
_ -> ()

as could this

match e with
_ -> true
| exception Not_found -> false

and this

let _ = e in e'

since all of these last three have the problem reported in this PR: if 'e' is a unintended partial application then the program is silently accepted. In each case adding an ascription can prevent the problem.

@vicuna
Copy link
Author

vicuna commented Jun 1, 2015

Comment author: @gasche

I like your suggestion. People could use (_ : _) to silence the warning if they insist on living the dangerous life, or (_ : _ -> _) to concisely indicate intent.

@vicuna
Copy link
Author

vicuna commented Feb 24, 2017

Comment author: @damiendoligez

Why not simply warn when matching a function type? This is almost always a mistake.

@github-actions
Copy link

github-actions bot commented Apr 5, 2021

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 Apr 5, 2021
@github-actions github-actions bot closed this as completed May 7, 2021
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