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

Warn about partial application on let _ = (arrow type) #1974

Merged
merged 2 commits into from Nov 17, 2018

Conversation

Projects
None yet
@nojb
Copy link
Contributor

commented Aug 7, 2018

In my experience (and others, judging by the discussion in #1971), let _ = e is a source of hard-to-track bugs.

This PR proposes to warn about partial application if e is of arrow type.

# let _ = Array.get;;
Warning 5: this function application is partial,
maybe some arguments are missing.
- : 'a array -> int -> 'a = <fun>

This is just a proof-of-concept; improvements may be possible with input from those more familiar with the type-checker.

@Armael

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

I think that's a good idea!

          match (expand_head exp_env exp_type).desc with
          | Tarrow _ ->
              Location.prerr_warning exp_loc Warnings.Partial_application
          | Tvar _ ->
              add_delayed_check (fun () -> check_application_result exp_env false exp)
          | _ -> 
             ...

After looking around briefly, this code pattern seems to be already copy-pasted in several places in typecore; and your patch would add yet an other occurrence. Do you think it is possible to instead introduce an auxiliary function for this?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

After looking around briefly, this code pattern seems to be already copy-pasted in several places in typecore; and your patch would add yet an other occurrence. Do you think it is possible to instead introduce an auxiliary function for this?

Yes, I noticed this too. I am planning to do the refactoring shortly.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

I'm a bit skeptical, because let _ = is to my knowledge the only language construct today to say "please really ignore". For example, if you get the "unused identifier foo" warning, you can always fix it with let _ = foo -- and most of the time foo will be a function.

It's unfortunate that people have taken the habit to use let _ = when they do not mean "please really ignore", but removing the language feature that lets us express this intent is a bit unfair to the people who are using it correctly.

I wonder if we could test whether the _-bound thing is a value or not. The use-cases I have in mind are mostly all specific to let _ = <identifier>, I don't know about use-cases with a non-value that is not of unit type.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

Not that I totally disagree with @gasche.

It might be better to start a propaganda machine for let () = ...; each time is see a let _ = main ... in sample code I am dismayed. However:

For example, if you get the "unused identifier foo" warning, you can always fix it with let _ = foo -- and most of the time foo will be a function.

Unless I'm mistaken there is actually a simpler (one character insertion) way to fix it: prefix the unused identifier with _. So maybe the case for this PR could still be made.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

Yeah, I agree -- in the majority of cases, I suspect just changing the identifier name so it starts with an underscore is enough.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

Personally I always use the underscore to signal "unused", and can't remember a situation when that was not enough.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

I use _<ident> to signal my intent to never use this identifier, but give it a name for readability reasons. When I do plan to use an identifier, but it's not currently used in the code (because it's work in progress), I use let _ = ident in ... or let _ = (id1, id2, ...) instead. In particular, I never use _<ident> for toplevel bindings -- I remove them or temporarily ignore then, but I don't rename them.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

If we had a standardized linter (for example ocp-lint), I think it would make sense to add a "code smell" rule for let _ = expr bindings where expr is not a syntactic value (open value, including identifiers), encouraging users to either eta-expand or use let () = instead.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

At janestreet we have a ppx linter which can, in particular, require all ignores (that is ignore but also let _ = ...) to be annotated.

I don't have much feedback regarding the usefulness / annoyance ratio, because I don't work on a part of the codebase where it is enabled, but hopefully someone will step in (I sent an email internally to direct some people towards this pr) to talk about it a bit more.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

@gasche You could just rename toplevel bindings to start with an underscore though. What is the rationale for not doing so?

@Armael

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

(I'm not so fond of let _ = ... and ignore ... having subtly different behaviors -- in fact until today I thought they were equivalent.)

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

It feels wrong. I can come up with reasons not to do it, one of them being that it doesn't work well if it is a recursive definition. The deeper reason is that _foo is for things that we intend not to use, and not for things that are currently not used. (I think it's useful to distinguish the two.)

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

I suspect people have different interpretations of your deep reason. I certainly do (I use underscores sometimes to keep code around which will be re-enabled later).

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

Another shallow reason is that _<identifier> doesn't work for identifiers that must be uppercase, like variant constructors.

@jvillard

This comment has been minimized.

Copy link

commented Aug 8, 2018

There's also the option of let[@warning "-32"] i_am_unused ..., or even

let i_am_unused () = ()
let[@warning "-5"] _ = i_am_unused
@gasche

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

(I'm still against the PR as proposed for the reasons that I gave earlier.)

let _ : _ -> _ = Genarray.get in
let _ : _ -> _ = Array1.get in
let _ : _ -> _ = Array2.get in
let _ : _ -> _ = Array3.get in

This comment has been minimized.

Copy link
@gasche

gasche Aug 8, 2018

Member

meh.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2018

(I'm still against the PR as proposed for the reasons that I gave earlier.)

Yes, that was my understanding. I am leaving this open for now to collect more feedback; in my experience this leads to actual bugs that are hard to track, so I would be happy if we could find a suitable workaround (not the one proposed in this PR necessarily).

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

Do you have opinions about my proposal to restrict the warnings to non-values?

(Armaël's point that it is disturbing that let _ and ignore behave differently is interesting; maybe we should look for a way to unify the behavior of both constructs.)

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

I still think we should teach people to write let () = {ignore,main} ... it's a much more accurate and value oriented description of what is going on. But I understand this may be a lost cause.

(Armaël's point that it is disturbing that let _ and ignore behave differently is interesting; maybe we should look for a way to unify the behavior of both constructs.)

I don't find it as disturbing as this, I'm not sure it makes sense to equate let _ = with ignore: the _ "keyword" appears in many places in the language simply to say "I do not want to give a proper name to that entity".

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

@dbuenzli I agree. What about having a warning when let _ = is used on a unit value, asking people to write let () instead? It's turning the current PR on its head: this new warning would not detect any bug, but it would shove non-buggy uses into the right idiom, and hope that uniformity and mimetism would do the rest.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

Having a warning in the unit case seems fine too. However I still think there is a strong argument for something along the lines of the original intention of this PR. This causes real bugs, which are hard to find.

@jberdine

This comment has been minimized.

Copy link

commented Aug 8, 2018

I agree that there is a source of real bugs here. What about a variant of @gasche's suggestion to allow let _ = <value> without warning, but to also allow let _ : ty = <exp>. The second form would allow protecting against e.g. adding a parameter to f in case <exp> is f x y. I would say add these warnings in addition to suggesting let () = as discussed already.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

Another suggestion (due to Stephen Weeks) when we considered this at Jane Street was, if I remember correctly, to allow let _ : _ = ... without a warning. However there was some concern that this pattern might become the new norm, negating the benefit of the warning on let _ = ....

@yallop

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

At the moment let _ : _ = ... is allowed without a warning by this PR, too, since it checks for the exact pattern Ppat_any (i.e. not Ppat_constraint etc.).

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

Indeed, constraining the pattern will deactivate the warning.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

I'm a bit skeptical, because let _ = is to my knowledge the only language construct today to say "please really ignore"

I usually do:

let () = ignore [f]

as a work-around to avoid the warning in:

# let () = ignore f;;
Characters 16-17:
  let () = ignore f;;
                  ^
Warning 5: this function application is partial,
maybe some arguments are missing.

But perhaps one should aim at removing this warning (whose wording is wrong, btw, f is not a function application).

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

Historically, the warning on ignore comes from the fact it was intended to be used as a work around to the statement warning. I.e., turn

e1; e2

into

ignore e1; e2

when the result type of e1 is not unit.
Of course, we do not want to disable the partial application warning in that case, and making a special case for a compound construct such as let _ = ignore e seems strange.
We might want to fix the message, but not remove the warning, as a non-application is still potentially a forgotten application.

This also meant, as @gasche says, that let _ = e was still left as the only way to bind an expression with a function type without warning.
Since then I have always argued to write let () = in top-level definitions, and done so myself, but with little impact on others.

All in all, while I would have been strongly opposed to such a warning in the past, it is clear that we are going in the direction of making the language safer, and this appears to be a man-made weak point.

As to whether let _ : _ = e would become a new weak point, I don't think so.
Real partial applications are rare, and I wouldn't expect people to write it all over the place when let _ = e does the job in 99.9% of cases.
And this syntax seems light enough for the cases @gasche has in mind.
One can even write

;; f1, f2, f3 ;;

for such uses.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2018

We might want to fix the message, but not remove the warning, as a non-application is still potentially a forgotten application.

I'm not convinced by that. The main risk in my opinion is when calling a function for which we don't need the result in some call sites, and then the function is extended with more arguments, but the ignore "hides" the call sites that need to be adapted. This cannot happen with non-applications.

My proposal would be:

  • Do not emit the warning about partial application on non-applications.
  • Add a warning for let _ = ... (but not for let _ : ... = ...).

This also meant, as @gasche says, that let _ = e was still left as the only way to bind an expression with a function type without warning.

let () = ignore [e] works as well, no?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2018

My proposal would be:

  • Do not emit the warning about partial application on non-applications.
  • Add a warning for let _ = ... (but not for let _ : ... = ...).

The warning on let _ = ... would also be restricted to actual applications only?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2018

The warning on let _ = ... would also be restricted to actual applications only?

I was thinking about always warning on let _ = .... The goal of this construction is to ignore stuff, and I believe making it more explicit (let () = ignore ....) is better.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

In today's caml-devel meeting this PR was discussed and decided that

  • ignore e and let _ = e should be equivalent (in particular, there should be a partial application warning for let _ = e), and
  • the partial application warning should not be triggered when e is an identifier.

@nojb nojb force-pushed the nojb:warn_partial branch 2 times, most recently from acf2c28 to 106ddf8 Nov 6, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Updated and rebased this PR, so that the check on ignore e and let _ = e is the same: warning 5 is triggered if e is of function type, but not an identifier. Also added some tests.

Lastly, I separated the "partial application check" from the "statement check" into two different functions, as it simplified the overall logic.

Could someone review it? Maybe @Armael ?

let check_partial_application exp =
let rec f delay =
match (expand_head exp.exp_env exp.exp_type).desc, exp.exp_desc with
| Tarrow _, Texp_ident _ -> ()

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Nov 6, 2018

Contributor
Suggested change
| Tarrow _, Texp_ident _ -> ()
| _, Texp_ident _ -> ()

(even in the Tvar case, we don't need to register a delayed check when the expression in a Texp_ident)

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Nov 6, 2018

Contributor

Since the warning is about partial application, shouldn't one restrict it explicitly to cases of expressions that are/return partial application (possibly traversing Texp_let, Texp_match, etc). For instance, let _ = r.e or let () = ignore (fun _ -> ...) should perhaps not trigger the warning.

Also, when the expression has an explicit type annotation, it would make sense to disable the warning (as we already do when the annotation is on the pattern): ignore (foo bar : _ -> _).

This comment has been minimized.

Copy link
@nojb

nojb Nov 7, 2018

Author Contributor

I pushed some commits doing both of these things, together with some tests. Thanks!

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

In the old code, when both the "partial application" warning and the "should-be-unit" warning were applicable, the "partial application" warning took precedence. In the new code the "should-be-unit" warning takes precedence. E.g. in let f _ _ = () in f 1; ..., we get a "should-be-unit" warning on f 1.

Do we want to restore the old behaviour?

@nojb nojb force-pushed the nojb:warn_partial branch from 7754b50 to 247bd0a Nov 8, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

The regression has been fixed. This is now ready for review.

@garrigue
Copy link
Contributor

left a comment

Looking at my notes from the developer's meeting, it looks like I was the one supposed to do that.
Since you already had an advance PR, I'm happy to leave that to you.
However, please fix the problems I pointed.

| Texp_ifthenelse (_, e, _)
| Texp_match (_, {c_rhs = e; _} :: _, _) ->
loop e
| _ ->

This comment has been minimized.

Copy link
@garrigue

garrigue Nov 16, 2018

Contributor

In the try, ifthenelse and match cases, e could a non-returning expression (raise something, ...), the non-unit type coming from another branch. So I'm not sure it's a good idea to recurse in those cases.

This comment has been minimized.

Copy link
@garrigue

garrigue Nov 16, 2018

Contributor

Looking below, it also seems that you should add let exception and let module here if you want to be complete.

This comment has been minimized.

Copy link
@nojb

nojb Nov 16, 2018

Author Contributor

Indeed, it's been fixed (both points).

| Texp_letexception _ | Texp_assert _ | Texp_lazy _ | Texp_object _
| Texp_pack _ | Texp_unreachable | Texp_extension_constructor _
| Texp_ifthenelse (_, _, None) | Texp_function _ | Texp_send _ ->
check_statement ()

This comment has been minimized.

Copy link
@garrigue

garrigue Nov 16, 2018

Contributor

If I understand correctly, the above are supposed to be cases which cannot be function applications, and as a result there is no need to warn for partial application.
However, the let exception and let module have the type of their body, which may be a function application.
Similarly, Texp_send denotes a method call, and Texp_new the creation of an object, which both amount to function applications, and should raise a warning. In my opinion Texp_field is also in that category when it returns a function type.

This comment has been minimized.

Copy link
@nojb

nojb Nov 16, 2018

Author Contributor

I have added the let exception and let module cases, and fixed the handling of Texp_send and Texp_new, thanks.

The case of Texp_field was discussed above and it was suggested that it should not raise an exception, which seems reasonable to me by analogy with the case of identifiers.

@nojb nojb force-pushed the nojb:warn_partial branch from 1f60c59 to 0e3ba1c Nov 16, 2018

@garrigue
Copy link
Contributor

left a comment

All seems fine now.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2018

Thanks, and sorry for the confusion!

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2018

I added one more commit fixing the location of the "statement" warning in the case of a type constraint (e : t) so that the whole type constraint is highlighted instead of just e.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2018

(the changed location in this case was a regression in this PR)

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

Is the history bisectable, or do you plan to rebase or squash?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2018

Not really, I will squash everything in two or three commits before merging.

@nojb nojb force-pushed the nojb:warn_partial branch from 52a972e to 9547a4d Nov 17, 2018

@nojb nojb merged commit 8145641 into ocaml:trunk Nov 17, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nojb nojb deleted the nojb:warn_partial branch Nov 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.