I've re-reviewed the PR.

I feel that the recommendation such as this one:

(** {b Concurrency safety} *)

[@@@alert concurrency_unsafe
    "Concurrent use of buffers is a programming error."

is too coarse-grained and suffers the risk of misinterpretation in multiple ways. It is unclear that the intended meaning of "concurrent use" is "accessing a buffer from multiple domains without a mutex protecting the buffer". In the parallel programming literature, "concurrent use" does not mean necessarily mean accessing without a mutex. I also agree with the earlier comment that "thread-safety" is not an appropriate term given that we have 3 kinds of threading -- domains, systhreads and user-level threads. The challenge here is that we do not have a widely accepted terminology for ready use. I don't have a good answer to this. What do you think @gasche @fpottier @stedolan?

What is the purpose of disabled-by-default alerts? When are the users likely to encounter these? If these are disabled by default, is their purpose to serve as documentation to be read by the users in the API docs? I don't know how alerts work in the compiler. Does building libraries using dune with warnings are errors trigger these alerts?

Error as warnings don't affect alerts. The intended purpose of the alerts was both for documentation purpose and to give users the option to selectively enable the alert in a limited scope.

After discussing with @Octachron privately, I agree that trying to come up with precise documentation here is not the goal.
The original motivation of the PR is to warn the users that OCaml 5.0 somehow does not make all of these standard library modules safe for use with parallelism and concurrency. With that in mind, the current PR seems ok to me.

Re. "concurrent use": one could say "racy concurrent use" (precise for people familiar with concurrent races, weird for everyone else), or "non-synchronized concurrent use", or "unsafe concurrent use".

"Unsynchronized concurrent access" would also work

What do you think @gasche @fpottier @stedolan?

I would vote for "unsynchronized concurrent access".

Unsynchronized concurrent access sounds good to me since it emphasizes that synchronization is left to the users of the mutable data structures.

You have an unsynchronized concurrent access here feels a bit long winded to me for discussing things.

What about establishing (un)synchronized access ? That's the kind of vocabulary used by Java™.

What about establishing (un)synchronized access? That's the kind of vocabulary used by Java™.

This sounds good to me.

"(Un)synchronized access" is also the term we use when teaching Concurrent & Distributed Systems in Cambridge. The "concurrent" is dropped as synchronisation is rarely useful in a sequential setting (with callback-driven code being the exception).

I like that the unsynchronized access vocabulary puts the emphasis on the root issue and give towards potential solution.
For the alert name, I am thinking of unsafe_unsychronized_access (or we could also drop the alert).

unsafe_unsychronized_access

We can go with unsynchronized_access. The term "unsynchronized" already has a negative connotation and hence I'd think that the prefix "unsafe" is not necessary.

I don't know: a more explicit name for the alert would unsafe_access_if_unsynchronized.

I fear that with unsynchronized_access the alert may give the impression to warn against a non-synchronized which is not an information that a alert can ever know.

Maybe a better shorter name would be unsafe_if_unsynchronized?

Hmm. In a concurrent setting, which is where the term unsynchronized makes sense, unsynchronized access is always unsafe. So unsafe_if_unsynchronized reads like a tautology to me.

Taking a step back, what is the purpose of the alert? Is it to enable users to optionally enable this alert to discover whether they are using any "concurrency unsafe" code? Presumably, the users are enabling this alert for their concurrent or parallel code. In this case, concurrency_unsafe seems like the right name for the alert. (we seem to have come full circle).

The workflow that I am considering for the alert would look like:

• enable the alert in a module with some shared mutable state
• for each alert:
  • disable the alert locally for false positives
  • fix the code otherwise

But then, outside of false positives, the unsynchronized_access name makes sense.

I think that I was trying too hard to harden the name against the false positive cases by taking in account that when enabling the alert on a new module, there will always be some false positives.

I would thus propose to go with unsynchronized_access (to avoid splitting the terminology).

Sounds good to me. Let's go with unsynchronized_access.

Cherry-picked on 5.0 as 616fa13 .

I get this alert in a CI build on macOS while running ocamldoc: https://github.com/ocaml-multicore/ocaml-tsan/actions/runs/3751404038/jobs/6372347665
Is this expected?

@gadmm made an excellent point on Discuss: when using a datastructure, it is not necessarily obvious which operations are read-only and which operations may mutate the structure, and this information is necessary to reason about whether a given usage is racy. (Read/read operations in parallel are not racy.)

I think that we should consider documenting explicitly which operations act as read-only on our datastructures. For example:

• for Hashtbl, find* and mem are read-only
• for Stack, top* is read-only
• for Stack and Queue, iterators are read-only
• Hashtbl iterators are currently not read-only (I wouldn't have guessed without reviewing the implementation)
• for Lazy values, Lazy.force thunk is generally not read-only, but I believe that it is read-only when Lazy.is_val thunk holds

Hashtbl iterators are currently not read-only (I wouldn't have guessed without reviewing the implementation)

This seems like it should be fixed. It looks like a possibly premature optimization in the code.

There is something that I don't understand about the unsynchronized_access alert:

$ ocaml -alert +all
OCaml version 5.0.0
Enter #help;; for help.

# !Sys.interactive;;
Alert unsynchronized_access: Stdlib.Sys.interactive
The interactive status is a mutable global state.
- : bool = true
# Buffer.create 42;;
- : Buffer.t = <abstr>
# Queue.create ();;
- : '_weak1 Queue.t = <abstr>

Buffer and Queue have an @@@alert annotation in their .mli (added by the present PR), so I would think that any usage of one of their functino should warn just as Sys.interactive (which has an item-specific annotation instead of a module-global annotation), right?

I feart that this is a rebasing mistake on my side: the alert should also be added to the module aliases exported by the Stdlib module (and that was done in a previous version of this PR that enabled the alert in the compiler before disabling it).

Ah, so this is basically #11867 making our life painful.