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

In core_type, [@attr] should bind less tightly than * #6612

Closed
vicuna opened this Issue Oct 12, 2014 · 13 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

vicuna commented Oct 12, 2014

Original bug ID: 6612
Reporter: @whitequark
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2016-12-07T10:47:24Z)
Resolution: fixed
Priority: normal
Severity: minor
Target version: 4.02.2+dev / +rc1
Fixed in version: 4.02.2+dev / +rc1
Category: ~DO NOT USE (was: OCaml general)
Monitored by: @Drup @gasche @diml @hcarty

Bug description

Right 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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 13, 2014

Comment author: @alainfrisch

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 13, 2014

Comment author: @gasche

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)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 13, 2014

Comment author: @damiendoligez

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 13, 2014

Comment author: @gasche

Well, I'd like to have both.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 13, 2014

Comment author: @whitequark

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])".

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 13, 2014

Comment author: @whitequark

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]"

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jan 29, 2015

Comment author: @diml

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jan 29, 2015

Comment author: @alainfrisch

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

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jan 30, 2015

Comment author: @diml

OK, I've started working on it:

https://github.com/diml/ocaml/tree/attribute-scope-improvement

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 9, 2015

Comment author: @alainfrisch

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!).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 9, 2015

Comment author: @diml

The part for types is working. I just need to update the printer and I'll submit it.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 9, 2015

Comment author: @diml

#152

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Mar 11, 2015

Comment author: @alainfrisch

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.

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.