Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0006612OCaml~DO NOT USE (was: OCaml general)public2014-10-12 23:492016-12-07 11:47
Reporterwhitequark 
Assigned Tofrisch 
PrioritynormalSeverityminorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version4.02.2+dev / +rc1Fixed in Version4.02.2+dev / +rc1 
Summary0006612: In core_type, [@attr] should bind less tightly than *
DescriptionRight now, the following type:

   int * int list [@split]

is parsed as:

   (int* ((int list)[@split ]))

I find that this is not what I expect and not what would be convenient for essentially any ppx rewriters, so I think the precedence should be changed.

This is potentially a breaking change, but I fully expect all non-buggy input to be parenthesized already.
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0012363)
frisch (developer)
2014-10-13 09:53

I'm not sure about which is the most "natural" precedence.

If we go with your proposal, we'd have a technical problem with:

 type t = A of int * int [@split]

since attributes cannot be attached to the argument list itself, only to individual arguments.
(0012364)
gasche (administrator)
2014-10-13 10:51

A good fix for the technical problem would be to allow

  type t = (A of int) | (B of string)

(parentheses should be allowed wherever they don't introduce conflicts)
(0012366)
doligez (administrator)
2014-10-13 11:31

> type t = (A of int) | (B of string)

I think you've got it backward. What you want is:

  type t = A of int * (int [@split]);;

which happens to be accepted already.
(0012368)
gasche (administrator)
2014-10-13 13:20

Well, I'd like to have both.
(0012372)
whitequark (developer)
2014-10-13 17:36

Hm, true. The issue is that specifying attributes that pertain to the constructor or the record field feels more natural when they're specified at the end of the line--which happens to attach them to an odd part of the type.

Consider these cases, written with a real use-case in mind and formatted as they probably would in a real codebase:

    type r = {
      f1 : int [@default 1];
      f2 : int * int [@default 10] [@drop_if (=) (1, 1)];
    }

    type v =
    | A [@value 1]
    | B of int [@default 1]
    | C of int * int [@default 1]
                     [@printer fun fmt -> fprintf fmt "%d %d"]

The problems here are:
  * f2 and C have the attributes attached to the wrong nodes
  * A and B, C have attributes attached to different nodes: constructor itself and the type

Arguably, transforming it into the following form would fix the issue:

    type r = {
      f1 [@default 1]
         : int;
      f2 [@default 10] [@drop_if (=) (1, 1)]
         : int * int;
    }

    type v =
    | A [@value 1]
    | B [@default 1]
        of int
    | C [@default 1] [@printer fun fmt -> fprintf fmt "%d %d"]
        of int * int

However, I argue that this makes the code much less readable, and that this breaks expectations of people writing code with attributes: that an attribute should be attached immediately after the field/constructor definition.

I propose a solution: amend the parser so that for fields, variant and polymorphic variant constructors, the attributes immediately following the type would be attached to the field or constructor itself. In order to attach them to the type, you have to parenthesize the type and attributes explicitly like "A of (int [@value 1])".
(0012373)
whitequark (developer)
2014-10-13 17:43

If downstream users (currently: only ppx_deriving as far as I'm aware) will, for the transition period, look at both the attributes of the field/constructor and of the core_type, the impact to the users can be made minimal. Indeed, the only change in such case would be the change of meaning of "X of int * int [@attr]"
(0013188)
dim (developer)
2015-01-29 14:44

To give more feedback to this issue, at Jane Street we extensively use record field annotations with type_conv. See for instance:

  https://github.com/janestreet/jenga/blob/master/examples/js-build-style/jengaroot.ml#L2735 [^]

I asked around and the feeling is that the record field annotation should indeed be at the end of the line. It kind of matches the common idiom [name : type = value].

Annotating a sub-type is very rare so it doesn't matter if it requires extra parenthesis.

More generally it would be clearer if the scope of a simple annotation (one @) would extend as far left as possible. It would make it clear where and when parentheses are needed, like for the [as] keyword. At the moment the scope depends on what operators are being used.
(0013189)
frisch (developer)
2015-01-29 14:48
edited on: 2015-01-29 14:48

I'm open to any such changes, and the sooner they are done, the better. Don't hesitate to propose patches.

(0013199)
dim (developer)
2015-01-30 17:38

OK, I've started working on it:

  https://github.com/diml/ocaml/tree/attribute-scope-improvement [^]
(0013411)
frisch (developer)
2015-03-09 10:09

dim: what's the status of your branch? Do you think it is ready for review and inclusion? I'd like to push this for 4.02.2 (with no guarantee!).
(0013413)
dim (developer)
2015-03-09 11:11

The part for types is working. I just need to update the printer and I'll submit it.
(0013416)
dim (developer)
2015-03-09 12:10

https://github.com/ocaml/ocaml/pull/152 [^]
(0013443)
frisch (developer)
2015-03-11 15:58

Thanks! Committed to trunk (rev 15897) and a modified version committed to 4.02 (rev 15896). Damien will monitor how much code this breaks on OPAM and decide to keep this for 4.02.2 or not.

- Issue History
Date Modified Username Field Change
2014-10-12 23:49 whitequark New Issue
2014-10-13 09:53 frisch Note Added: 0012363
2014-10-13 10:51 gasche Note Added: 0012364
2014-10-13 11:31 doligez Note Added: 0012366
2014-10-13 11:31 doligez Target Version => 4.02.2+dev / +rc1
2014-10-13 13:20 gasche Note Added: 0012368
2014-10-13 17:36 whitequark Note Added: 0012372
2014-10-13 17:43 whitequark Note Added: 0012373
2014-11-18 19:02 doligez Status new => acknowledged
2015-01-29 14:44 dim Note Added: 0013188
2015-01-29 14:48 frisch Note Added: 0013189
2015-01-29 14:48 frisch Note Edited: 0013189 View Revisions
2015-01-30 17:38 dim Note Added: 0013199
2015-03-09 10:09 frisch Note Added: 0013411
2015-03-09 11:11 dim Note Added: 0013413
2015-03-09 12:10 dim Note Added: 0013416
2015-03-09 12:19 frisch Assigned To => frisch
2015-03-09 12:19 frisch Status acknowledged => assigned
2015-03-11 15:58 frisch Note Added: 0013443
2015-03-18 13:52 frisch Status assigned => resolved
2015-03-18 13:52 frisch Fixed in Version => 4.02.2+dev / +rc1
2015-03-18 13:52 frisch Resolution open => fixed
2016-12-07 11:47 xleroy Status resolved => closed
2017-02-23 16:36 doligez Category OCaml general => -OCaml general
2017-03-03 17:55 doligez Category -OCaml general => -(deprecated) general
2017-03-03 18:01 doligez Category -(deprecated) general => ~deprecated (was: OCaml general)
2017-03-06 17:04 doligez Category ~deprecated (was: OCaml general) => ~DO NOT USE (was: OCaml general)


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker