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

Add a warning on type declarations "type t = ()" #2091

Merged
merged 2 commits into from Oct 6, 2018

Conversation

Projects
None yet
5 participants
@Armael
Copy link
Member

commented Oct 4, 2018

This adds a warning for an amusing mistake which managed to make @linse, @hannesm and @samoht confused for a bit of time this afternoon at the mirage retreat!

Writing a type declaration type foo = () instead of type foo = unit defines a new type foo with constructor (), which now breaks random parts of the code with surprising error messages, since () now denotes two different incompatible constructors (as a constructor of foo, and as a constructor of unit).

I think it most (all?) cases, a declaration type foo = () is a mistake, so here's a warning for it.

let open Parsetree in
let is_unit_constructor : constructor_declaration -> bool = function
| { pcd_name = { txt = "()" };
pcd_args = Pcstr_tuple [];

This comment has been minimized.

Copy link
@Octachron

Octachron Oct 4, 2018

Contributor

I am not sure if it is useful to not warn on type t = ( ) of int .

This comment has been minimized.

Copy link
@Armael

Armael Oct 4, 2018

Author Member

My initial reasoning was that if you're doing something very strange like this, then there must be a reason.. In the other hand, if we can't think of a such reason, then we might as well warn on it indeed.

This comment has been minimized.

Copy link
@Armael

Armael Oct 4, 2018

Author Member

I amended the code to also warn on these cases (and also stuff like type _ t = () : int foo :-).

in
match td with
| { ptype_name = { txt = name };
ptype_params = []; ptype_cstrs = []; ptype_manifest = None;

This comment has been minimized.

Copy link
@Octachron

Octachron Oct 4, 2018

Contributor

Similarly, are

type 'a t = ( )
type  'a t = ( ) constraint 'a = int

that different from type t = ( ) ?

@Armael Armael force-pushed the Armael:redefine_unit branch 2 times, most recently from 9240b66 to 2f75edb Oct 4, 2018

@gasche
Copy link
Member

left a comment

This sounds like a good idea. A few comments:

  • Warnings are documented in -warn-help, is this properly updated by this patch?
  • I don't like the phrasing "defining an alternative 'unit' type" so much, it feels like jumping to conclusions about what the user is doing; what do you think of "introducing a new constructor named '()'" instead?
@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

Warnings are documented in -warn-help, is this properly updated by this patch?

It isn't, will do.

what do you think of "introducing a new constructor named '()'" instead?

Why not. Nitpicking, but this does not exactly describe what is currently implemented. Currently, the warning is triggered if the type declaration introduces exactly one new constructor named (). Do you think the warning should be extended to stuff like type t = () | Foo? This sounds like a reasonable thing to do...

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

Although it makes it harder to display a hint...

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

While I can imagine a programmer write the first use cases thinking this is only an alias for unit, if she writes a real datatype declaration (with multiple constructors) then she has to know that () is really a constructor...

@Armael Armael force-pushed the Armael:redefine_unit branch from 2f75edb to 0b3d047 Oct 5, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

True. I amended the PR, implementing gasche comments, and without generalizing to type t = () | ... as per @garrigue remark.

@Armael Armael force-pushed the Armael:redefine_unit branch from 0b3d047 to 51007b8 Oct 5, 2018

@gasche
Copy link
Member

left a comment

I was going to approve and say "but add a comment at the bottom of warnings.ml:type t to ask people to update the description as well". I went to check that the top that there wasn't a similar comment. There is, and it talks about something else: you also have to update manpages man/ocamlc.m (Edit: no need to update ocamlopt.m in fact, it refers to ocamlc.m).

Hopefully one day this part of ocamlc.m can be auto-generated by a script. I recently did this for ocamlbuild.

(I'm not sure why it also mentions the manual, as far as I can tell there is no per-warning documentation in the manual, only a recent Warning reference section that only comments on some specific warnings.)

Armael added some commits Oct 4, 2018

Add a warning triggered on type declarations "type t = ()"
These are most likely a mistake for "type t = unit", but still valid, and lead
to quite confusing error messages afterwards because now `()` denotes two
different incompatible constructors.

@Armael Armael force-pushed the Armael:redefine_unit branch from 51007b8 to 436a929 Oct 5, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

Argh, indeed. It seems only man/ocamlc.m needs to be updated though. I changed the comment at the top of warnings.ml to reflect that.

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

Just to make sure: with your patch, the new warning is enabled by default, right?

(I think this is the right choice and I think this is what the code does, but this choice was never mentioned explicitly.)

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

Correct. This is because the default set of warnings is defined as "+a-X-Y-Z" so all new warnings get enabled by default.

@gasche

gasche approved these changes Oct 5, 2018

@gasche gasche merged commit 494af09 into ocaml:trunk Oct 6, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

FTR, we should resurrect MPR#5936

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.