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 paths for built-in types #1876

Merged
merged 7 commits into from Nov 8, 2018

Conversation

Projects
None yet
8 participants
@yallop
Copy link
Member

commented Jul 2, 2018

It's often useful to write or generate code that refers to built-in types such as int or option, but it's currently inconvenient to do so in a way that avoids shadowing.

This PR adds aliases in Stdlib for the built-in types option, int, bool, unit, and exn, and aliases in List and Array for the built-in types list and array. Other built-in types (floatarray, bytes, string, float, nativeint, int32, int64, lazy_t and char) are already aliased elsewhere in the standard library.

At some point in the future there may be standard library modules for some of these types (e.g. Option or Bool); if that happens before the next release then the corresponding aliases in Stdlib could be removed.

(Fixes Mantis 5072: Absolute name for built-in types such as int, bool, ... ? and Mantis 6655: Alias built-in types and pre-defined exceptions in Pervasives.)

Show resolved Hide resolved stdlib/stdlib.mli Outdated
Show resolved Hide resolved stdlib/list.mli Outdated
@sliquister

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

Up to the couple of things I pointed out, the change does almost as advertised, the only exception being that the type extension_constructor is not aliased anywhere. Might be worth adding in Obj for consistency?
No strong opinion, but it could fine to alias all built-in type constructors in stdlib.mli, so that reading stdlib.mli tells you the initial scope. For this reason, I see no issue with having Stdlib.option and possibly in the future, Stdlib.Option.t.

I'll note that although this enable uses of the type constructors Stdlib.bool, the value constructors cannot be used in some cases, which theoretically limits what can be done without triggering warning 40 and 42 (which are enabled by default, even if I always disable them):

# type bla = [] | (::) | () | None | Some | false | true;;
type bla = [] | (::) | () | None | Some | false | true
# Stdlib.[];;
- : bla = [] (* wrong: scope issue *)
# let v = Stdlib.(false);;
val v : bool = Stdlib.false (* good, except the syntax "Stdlib.false" doesn't parse *)
# if true then false else Stdlib.(false);;
- : bla = false (* wrong: type based disambiguation apparently overrides module path *)
# if true then false else v;; (* good *)
Error: This expression has type bool but an expression was expected of type
         bla

And talking about constructors, the built in constructors of sum types are now available in the stdlib, but not the ones of exceptions (Not_found, Match_failure, Out_of_memory etc). I think you'd need to do this to claim to fix MPR#6655.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

I'm not sure what you mean by "wrong" in some of the examples you list; do you mean that the current semantics is wrong, or that the behavior is too surprising and we should warn about it, or something else?

Note that

# Stdlib.[];;
- : bla = []

comes from the fact, I think, that Jeremy re-exports [] in Stdlib.List, not in Stdlib itself. Since 1c4ee3a, which was merged in 4.04,M.[] is resolved as let open M in [] (where [] is handled as an identifier "[]" by the parser).

@sliquister

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

Oh well I expected []/true/false to be normal constructors. But you're saying they are not, because there is no syntax to access a prefixed version of them, and the syntax that looks like that is actually a local open, so that explains the weird scoping.
Alright, well nevermind this remark.

@yallop

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2018

Thanks for the thorough review, @sliquister. I've addressed your comments in the latest set of pushed commits.

A number of tests are currently failing due to changes in type printing. I plan to wait a little while for further comments on the existing change before addressing the tests to avoid churn.

@yallop

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2018

the type extension_constructor is not aliased anywhere. Might be worth adding in Obj for consistency

Done.

And talking about constructors, the built in constructors of sum types are now available in the stdlib, but not the ones of exceptions (Not_found, Match_failure, Out_of_memory etc). I think you'd need to do this to claim to fix MPR#6655.

Done.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

Another beneficial effect of this PR is that it solves the issue with the manual index MPR#7708 by giving an indexed path to built-in exceptions and types.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

One thing I hadn't commented on is: I'm not super happy about the not-real-constructors [], (), true and false showing up in actual OCaml code. It is an open secret that these parse, and some people that are too clever for their own good have used it in the wild already, but having this right in Stdlib may hide the fact that this is an undocumented, not-guaranteed-to-keep-working thing the parser currently does.

Could you maybe add a comment whenever this happen, to indicate that this is a hack to give a path in Stdlib to those constants, but similar tricks are not meant to be used in user code?

@yallop

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2018

@gasche

I'm not super happy about the not-real-constructors [], (), true and false showing up in actual OCaml code.
[...]
Could you maybe add a comment whenever this happen

I'm happy to add a comment for (), true and false. But [] is a real constructor nowadays, isn't it?

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

Not in my book (but I would be wrong if it was, for example, documented in the language reference); I thought of this PR more as an exercise in making compromises on the implementation details that helps advanced users that do not-really-supported things.

@yallop

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2018

The manual has:

constr-decl:
(constr-name || '[]' || '(::)') [ 'of' constr-args ]
@Drup

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

IMHO, if [] and :: are not documented nowadays, then it's a documentation failure, not an undocumented feature. The fact that we can redefine them is extremely useful in many cases.

If you feel like this should stay undocumented, but still want to fulfill the use case properly, you are welcome to champion the first commit in my patchset for infix constructors. :)

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

For the record, I am rather in favor of integrating (::) and your infix constructors in the language. My impression, however, was that there was no consensus on that, and that user-defined lists were more of a tolerated accident. I appear to be wrong, though, as [] and (::) are documented. Maybe I should ask around to know what people feel now.

@Drup

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

I would be happy to resurrect my patchset (or even a subset of it) if you feel the weather is more favorable.

stack reaches its maximal size. This often indicates infinite or
excessively deep recursion in the user’s program. (Not fully
implemented by the native-code compiler.) *)

This comment has been minimized.

Copy link
@sliquister

sliquister Jul 2, 2018

Contributor

This description is a bit weird "exception raised by bytecode runtime. Not fully implemented in native compiler". The first sentence should probably be runtime-agnostic and then, the last sentence can say that this is best-effort in the native runtime.

This comment has been minimized.

Copy link
@sliquister

sliquister Jul 3, 2018

Contributor

Oh I realized you took the description from the manual. Well the manual is weird then, but it's probably not worth diverging.

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Oct 29, 2018

Contributor

Currently, the manual has a dedicated chapter for built-in types and predefined exceptions. Does it make sense to keep it if they are all defined in the Stdlib? Or perhaps just pointers to reexport definitions? We should avoid text duplication is possible.

More general question: do we want to keep those predefined exceptions as part of the global scope even if Stdlib is not opened? Maintainers of stdlib alternatives might want to hide, rename or put in different sub-modules some of those exceptions. I guess that exceptions are predefined because they are required by the runtime system (and relying on OCaml code to register them is risky if the full stdlib is not linked); but perhaps one could have a syntax to refer to internal exceptions without "polluting" the global scope:

exception Stack_overflow = Stack_overflow[@@ocaml.builtin];;

Do you think it is worth the trouble?

@sliquister
Copy link
Contributor

left a comment

The code/documentation looks good, I think the only concern is whether the warning on Failure "int_of_string" and friends is broken.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

About the weird constructors: it's probably time to resume working on MPR#5936. Please have a look at the discussion there.

@alainfrisch alainfrisch added the stdlib label Jul 12, 2018

Show resolved Hide resolved stdlib/stdlib.ml Outdated
Show resolved Hide resolved stdlib/stdlib.ml Outdated
Show resolved Hide resolved stdlib/stdlib.mli Outdated

@yallop yallop force-pushed the yallop:builtin-type-paths branch from 22deed2 to 8705a09 Oct 29, 2018

@yallop

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2018

@alainfrisch: I've removed the bool, int, and option aliases from Stdlib, and added true and false to the Bool module.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

@damiendoligez You expressed somewhere else your reluctance against adding more "toplevel" declarations to Stdlib. Can we argue that the components added here (types and exceptions) are already part of the "global scope" anyway and thus assume you are ok with those additions?

@yallop yallop force-pushed the yallop:builtin-type-paths branch from 8705a09 to 7c11ce5 Nov 3, 2018

@yallop

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2018

The code/documentation looks good, I think the only concern is whether the warning on Failure "int_of_string" and friends is broken.

This is now fixed, and I've rebased/squashed to clean up the history. I've also opened #2133 to make the @ocaml.warn_on_literal_pattern attribute more meaningful for multi-argument constructors and non-trivial patterns.

I think this is now ready for merge, if CI passes. A number of tests are failing, because of printing differences, like this:

-        val print : unit -> unit
+        val print : unit/1 -> Stdlib.unit

Suggestions on how best to address these failures are welcome. Some possibilities:

  1. Update the tests to use -short-paths, so that Stdlib.unit is always replaced with unit
  2. Add a Unit module and move the type alias there instead, so that the built-in type is not shadowed by default
  3. Special-case Stdlib in type-printing code so that aliases are always resolved.

My preference is for 2, but I'm happy to hear arguments for other approaches.

@yallop yallop force-pushed the yallop:builtin-type-paths branch 2 times, most recently from 903c886 to 1035527 Nov 3, 2018

@yallop

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2018

Update: I moved unit and exn out of Stdlib into their own modules (Unit and Printexc respectively).

Show resolved Hide resolved stdlib/array.ml Outdated
Show resolved Hide resolved stdlib/printexc.mli Outdated
Show resolved Hide resolved stdlib/obj.mli Outdated

@yallop yallop force-pushed the yallop:builtin-type-paths branch from 1035527 to 1cd0f5d Nov 5, 2018

Show resolved Hide resolved stdlib/obj.mli Outdated
@alainfrisch
Copy link
Contributor

left a comment

LGTM. Waiting for a few days before merging in case someone else wants to add something.

@yallop yallop force-pushed the yallop:builtin-type-paths branch 2 times, most recently from 3f06285 to 190a974 Nov 5, 2018

@nojb nojb referenced this pull request Nov 6, 2018

Open

[RFC] Add Exn module #2137

@yallop yallop force-pushed the yallop:builtin-type-paths branch from 190a974 to 9c71d4c Nov 6, 2018

@alainfrisch alainfrisch merged commit ee1c2a4 into ocaml:trunk Nov 8, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

Thanks @yallop!

@yallop yallop deleted the yallop:builtin-type-paths branch Nov 19, 2018

@hhugo

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

The module [Unit] needs to be exported in [stdlib.ml]

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.