disable warning 30 (same constructor/label used in two mutually-recursive declarations)#9046
Merged
gasche merged 1 commit intoocaml:trunkfrom Oct 18, 2019
Merged
Conversation
dfaa169 to
92a260f
Compare
…sive decls)
Warning 30 warns when the same constructor/label is used in two
mutually-recursive type declarations. This warning (added in OCaml
3.12, 2010) was meaningful at the time where constructor/label
conflicts had a shadowing semantics: the "last" definition would mean,
while ideally mutually-recursive definitions should not be strictly
ordered but defined "all at once".
Since OCaml 4.01 (2013) we support type-based disambiguation of
constructor/label names and it is now common to write code such as,
say
type pattern = Var of var | ...
type expr = Var of var | ...
(no warning, of course). But warning 30 fires if you instead write
type pattern = Var of var | ...
and expr = Var of var | ...
This doesn't make much sense, and in particular it certainly makes no
sense to enable this warning by default. The present PR disables it by
default.
Member
Author
|
@Drup if you like the PR, would you mind approving it? (Or maybe you like it but you are not sure it should be merged?) |
Member
Author
|
In general for warnings we would have to be fairly careful with the default settings (they affect the language model and require Serious Consensus to be changed), but in this particular I'm pretty sure that everyone just forgot about this warning and it's a mistake that it slipped through unnoticed so far. (Probably because most existing codebases didn't start using same-constructor-names overnight so we didn't notice it.) The reason I created this PR now is that a new intern of mine just wrote code that fell on this warning, and that was a bad user experience. I thus propose to move on and merge as the CI is green. @trefis, does that sound reasonable to you? |
Contributor
|
It does. Although if you want to be extra careful, you could try and confirm that your explanation is correct by digging into old issues / blaming the code and looking at the commit message (or asking its author). |
Member
Author
|
Ah, I did the digging part already: MPR#3601 in 2010. |
gasche
added a commit
to gasche/dune
that referenced
this pull request
Dec 26, 2023
Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10 ocaml/ocaml#9046 and there is no reason to keep it in Dune. Signed-off-by: Gabriel Scherer <gabriel.scherer@gmail.com>
gasche
added a commit
to gasche/dune
that referenced
this pull request
Dec 26, 2023
Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10 ocaml/ocaml#9046 and there is no reason to keep it in Dune. Signed-off-by: Gabriel Scherer <gabriel.scherer@gmail.com>
gasche
added a commit
to gasche/dune
that referenced
this pull request
Dec 27, 2023
Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10 ocaml/ocaml#9046 and there is no reason to keep it in Dune. Signed-off-by: Gabriel Scherer <gabriel.scherer@gmail.com>
rgrinberg
pushed a commit
to gasche/dune
that referenced
this pull request
Dec 28, 2023
Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10 ocaml/ocaml#9046 and there is no reason to keep it in Dune. Signed-off-by: Gabriel Scherer <gabriel.scherer@gmail.com>
gasche
added a commit
to gasche/dune
that referenced
this pull request
Dec 28, 2023
Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10 ocaml/ocaml#9046 and there is no reason to keep it in Dune. Signed-off-by: Gabriel Scherer <gabriel.scherer@gmail.com>
gasche
added a commit
to gasche/dune
that referenced
this pull request
Jan 4, 2024
Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10 ocaml/ocaml#9046 and there is no reason to keep it in Dune. Signed-off-by: Gabriel Scherer <gabriel.scherer@gmail.com> Co-authored-by: Rudi Grinberg <me@rgrinberg.com>
rgrinberg
added a commit
to gasche/dune
that referenced
this pull request
Jan 8, 2024
Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10 ocaml/ocaml#9046 and there is no reason to keep it in Dune. Signed-off-by: Gabriel Scherer <gabriel.scherer@gmail.com> Co-authored-by: Rudi Grinberg <me@rgrinberg.com>
rgrinberg
added a commit
to gasche/dune
that referenced
this pull request
Jan 8, 2024
Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10 ocaml/ocaml#9046 and there is no reason to keep it in Dune. Signed-off-by: Gabriel Scherer <gabriel.scherer@gmail.com> Co-authored-by: Rudi Grinberg <me@rgrinberg.com>
rgrinberg
pushed a commit
to ocaml/dune
that referenced
this pull request
Jan 8, 2024
Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10 ocaml/ocaml#9046 and there is no reason to keep it in Dune. Signed-off-by: Gabriel Scherer <gabriel.scherer@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning 30 warns when the same constructor/label is used in two
mutually-recursive type declarations. This warning (added in OCaml
3.12, 2010) was meaningful at the time where constructor/label
conflicts had a shadowing semantics: the "last" definition would mean,
while ideally mutually-recursive definitions should not be strictly
ordered but defined "all at once".
Since OCaml 4.01 (2013) we support type-based disambiguation of
constructor/label names and it is now common to write code such as,
say
(no warning, of course). But warning 30 fires if you instead write
This doesn't make much sense, and in particular it certainly makes no
sense to enable this warning by default. The present PR disables it by
default.