Skip to content

Conversation

@sim642
Copy link
Member

@sim642 sim642 commented Dec 2, 2025

This is on top of #1880.

Instead of writing

f @@ fun x ->
e

(which is a bit odd indentaiton-wise), this provides a neat way to write it more idiomatically as

let@ x = f in
e

This particularly makes sense for various Fun.protect derivatives, like the file operations introduced in #1880 to not forget closing the file (even in the case of exceptions).

Such an operator already exists, e.g. in the containers library.

TODO

  • Figure out what to do about semgrep false positives.

@sim642 sim642 added this to the v2.8.0 milestone Dec 2, 2025
@sim642 sim642 added cleanup Refactoring, clean-up pr-dependency Depends or builds on another PR, which should be merged before labels Dec 2, 2025
Comment on lines +897 to +898
let@ () = GobRef.wrap AnalysisState.executing_speculative_computations true in
invariant man st exp tv

Check warning

Code scanning / Semgrep OSS

Semgrep Finding: semgrep.let-unit-in Warning

use ; instead (and, if needed, add surrounding parentheses to preserve precedence)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, looks like semgrep isn't particularly smart (again) and thinks let@ is just let...

Comment on lines +220 to +221
let@ () = GobRef.wrap AnalysisState.executing_speculative_computations true in
Idx.add bytes_offset remaining_offset

Check warning

Code scanning / Semgrep OSS

Semgrep Finding: semgrep.let-unit-in Warning

use ; instead (and, if needed, add surrounding parentheses to preserve precedence)
Comment on lines +233 to +234
let@ () = GobRef.wrap AnalysisState.executing_speculative_computations true in
Idx.mul item_size_in_bytes x

Check warning

Code scanning / Semgrep OSS

Semgrep Finding: semgrep.let-unit-in Warning

use ; instead (and, if needed, add surrounding parentheses to preserve precedence)
Comment on lines +237 to +238
let@ () = GobRef.wrap AnalysisState.executing_speculative_computations true in
Idx.add bytes_offset remaining_offset

Check warning

Code scanning / Semgrep OSS

Semgrep Finding: semgrep.let-unit-in Warning

use ; instead (and, if needed, add surrounding parentheses to preserve precedence)

Check warning

Code scanning / Semgrep OSS

Semgrep Finding: semgrep.let-unit-in Warning

use ; instead (and, if needed, add surrounding parentheses to preserve precedence)
Base automatically changed from channel-with to master December 2, 2025 13:39
@sim642 sim642 removed the pr-dependency Depends or builds on another PR, which should be merged before label Dec 2, 2025
@michael-schwarz
Copy link
Member

I find the let@ things hard to understand, but maybe that is just me?
Wdyt @karoliineh @DrMichaelPetter @arkocal?

@sim642
Copy link
Member Author

sim642 commented Dec 3, 2025

I've always found the @@ fun _ -> construct a bit strange because it's really like writing code in continuation-passing style which we never actually do.
It's also odd indentation-wise (and I think ocp-indent has just been hacked to be fine with it: OCamlPro/ocp-indent#188).
Because everywhere else we write funs as function arguments like

f (fun x ->
    e
  )

Note that there's a level of indentation coming from it just being in the function argument of f and another level coming from being the body of the fun.
In these places, to avoid unnecessarily deep indentation it has been written as

f @@ fun x ->
e

which tries to give the impression of linear code as opposed to going down into a scope (which it does).

And that's exactly the kind of code let binding operators were introduced for and have it just as

let@ x = f in
e

(Here the = f in only looks odd because it's a function without arguments. Actually in all of the uses there are arguments, like the file name being opened.)

This offers a very easy way to avoid file descriptor leaks. Instead of the buggy

let f = open "foo.txt" in
e

just do

let@ f = with_open "foo.txt" in
e

No need to touch/reindent e which may be a large block of code.

People have complained about such operator elsewhere as well (ocaml/ocaml#9887), but not for this reason at all. There nobody is advocating for the middle @@ fun x -> variant, but it's a battle between let@ and (fun x -> ...).
The middle variant has no advantages over either of the other two.

@DrMichaelPetter
Copy link
Collaborator

I find the let@ things hard to understand, but maybe that is just me? Wdyt @karoliineh @DrMichaelPetter @arkocal?

I will have to look it up, when I encounter it, but this is more due to my Ocaml illiteracy - same is the case for lots of other operators that I do not encounter on a daily base. It it helps to contain the file handle leak in an elegant way, then why not.

@karoliineh
Copy link
Member

I find the let@ things hard to understand, but maybe that is just me? Wdyt @karoliineh @DrMichaelPetter @arkocal?

As we already have the let+ and let*, which I've had to use, I don't really mind adding another similar construct. It's just when you don't know these exist, you might write it in the more clumsy way at first, but as the meaning of let@ is well documented in the definition, and if there is a semgrep rule for it, I don't mind -- as long as code looks cleaner after, I'm in favor.

@arkocal
Copy link
Contributor

arkocal commented Dec 3, 2025

I have no preference here, so I will just join the majority.

@sim642
Copy link
Member Author

sim642 commented Dec 3, 2025

if there is a semgrep rule for it, I don't mind

I'll have to see about the semgrep false positives before we can really do this anyway.

Having a semgrep rule to suggest let@ instead might also be nice, but it's not really useful if semgrep can't properly distinguish things at all.

@michael-schwarz
Copy link
Member

Alright, if everyone else is on board, let's do it!

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

Labels

cleanup Refactoring, clean-up

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants