Do not discard attributes attached to pattern variables by nojb · Pull Request #1822 · ocaml/ocaml

nojb · 2018-06-07T13:03:33Z

This PR adds the necessary plumbing so that attributes attached to variables bound in patterns are not discarded. It is motivated by MPR#7719, where it is observed that [@@deprecated] attributes attached to top-level lets are ignored.

For example, this PR makes it so that

let (_, foo[@deprecated], _) = .... in foo

will trigger the deprecation warning.

The PR is better reviewed commit-by-commit:

the first commit replaces the type Typecore.pattern_variable by a record type to aid readability;
the second commit keeps track of attributes attached to pattern variables in the pattern_variable type; and
the third commit records that information when the pattern variables are entered into the environment.

In order to fix MPR#7719 some kind of cascading logic is needed so that [@@deprecated] attributes attached to a let are inherited by the individual variables bounds in the pattern. But it wasn't clear to me that it was a good idea for this to be the case for arbitrary attributes. Maybe special casing some attributes (such as [@@deprecated]) would be a better idea.

trefis

I suggest you squash the second and third commit.
Apart from that everything looks good to me.

I also agree with your choice of not adding the suggested cascading logic in this PR. I think the current GPR should be mergeable fairly quickly and that further steps can be discussed on the original MPR.

trefis · 2018-06-07T13:23:11Z

typing/typecore.ml

+  {
+    pv_id: Ident.t;
+    pv_type: type_expr;
+    pv_name: string loc;


I believe the pv_name field is useless. Not only is it redundant with the pv_id and pv_loc fields, as far as I can tell it's only "used" to be stuffed in Tcl_fun, and then completely ignored when Tcl_fun is processed in translclass.ml.

This remark is somewhat orthogonal to your PR, but since you're cleaning up that area, might as well go all the way :)
Do you mind trying to add a commit which removes that field?

This is done, I removed the corresponding field from both Tcl_fun and Tcl_let, which resulted in a nice cleanup.

trefis · 2018-06-07T13:30:56Z

typing/typecore.ml

-      | [],(y,_,_,_,_)::_  -> raise (Error (loc, env, Orpat_vars (y, [])))
-      | (x,_,_,_,_)::_, (y,_,_,_,_)::_ ->
+      | {pv_id = x; _}::_, [] -> raise (Error (loc, env, Orpat_vars (x, [])))
+      | [],{pv_id = y; _}::_  -> raise (Error (loc, env, Orpat_vars (y, [])))


While you're touching these two lines (and I'm making stylistic comments), do you mind merging the two cases above?

xclerc · 2018-06-07T13:33:33Z

Unsure whether it falls under your notion of cascading,
but the following will not trigger the warning:

let (_, (foo : int)[@deprecated], _) = ...

while the following will:

let (_, (foo[@deprecated] : int), _) = ...

It might be surprising.

nojb · 2018-06-07T13:54:22Z

Thanks for the review @trefis! I will do the suggested changes.
@alainfrisch points out that in the case of an "or" pattern only the attributes of the leftmost pattern will be kept. What to do? Some possibilities:
- merge attributes attached to similarly-named variables in both branches of the "or" pattern
- keep only those attributes which are present and identical in both branches of the "or" (including payload)
- leave as it is, only attributes on the left of the "or" are kept, and document this behavior.

Ideas/suggestions?

nojb · 2018-06-07T13:56:33Z

Unsure whether it falls under your notion of cascading,

Indeed, this behavior may be surprising in the case of a [@deprecated] attribute, but it is not clear what is the right thing to do in general for an arbitrary attribute, so I am leaving that discussion for a later PR.

Drup · 2018-06-07T13:58:24Z

I had a look at the code as well, I agree with everything @trefis said.

I vote for option 4: Warn on deprecated attributes in non-nonsensical places. We already warn on nonsensical inline attributes, so there is a precedent.

gasche · 2018-06-07T14:06:40Z

The problem with warning is that if we warn in one version, and then we improve the behavior in the next version (eg. implement the reasonable cascading behavior), people that want their code to be warning-free on all recent versions will be unable to use the nice behavior.

I would rather give a bit more thought to how what is the desirable semantics that start to implement warnings from our current implementation right away. No objection of course for things that we believe do not actually make sense -- rather than things that are not currently supported.

trefis · 2018-06-07T15:30:51Z

Indeed, Alain is correct.
I'm not quite sure what the best option is. I'm also not sure whether @gasche and @Drup were talking about the or-pattern "problem" when they talked about warnings or not.

I was initially tempted to propose to warn when the set of attributes for a given variable is not the same in all branches. But I can easily imagine people being frustrated by having to repeat the information.
Still, silently dropping any attribute seems bad, even if it is documented.
So two options remain:

@Drup's proposal: warn on attributes which are not in the right branch
merge the sets of attributes

I find the second option more appealing but I'm not sure what the implementation would look like... That is: can we easily and reliably deduplicate attributes present in several branches?

trefis · 2018-06-07T15:32:43Z

Btw, I confess that 0d6569f brought me joy. I hope writing it felt immensely satisfying.

gasche · 2018-06-07T15:36:37Z

I think I would be in favor of merging the PR about as it is now. It is not perfect, but it is at a stable point that is strictly better than what we have before, and we can discuss later concerns after. The refactoring patches are good to have now and might be annoying to rebase later.

nojb · 2018-06-07T15:47:39Z

OK, let's merge this now and leave further improvements for later. I will merge once the CI passes unless someone beats me to it :)

alainfrisch · 2018-06-07T15:54:11Z

that is: can we easily and reliably deduplicate attributes present in several branches?

One certainly want to ignore all locations when comparing the attributes (including locations deeply nested in the payload); I believe this can be done easily using Ast_mapper.

But it would be the first case, I think, where we compare Parsetree fragments (using structural equality). I think the use of structural equality should be ok, technically, but is that a good semantics?

Yet another solution would be to merge without deduplication: this is straightforward to implement and document. This might interact badly with a future addition of implicit cascading, but:

It might not be so bad anyway. E.g. having multiple deprecated attributes is harmless (only one will be reported anyway).
If needed, one could special case "cascaded" attributes to avoid duplication induced only by cascading (i.e. add cascaded attributes on the way up).

lpw25 · 2018-06-07T16:03:56Z

In order to fix MPR#7719 some kind of cascading logic is needed so that [@@deprecated] attributes attached to a let are inherited by the individual variables bounds in the pattern. But it wasn't clear to me that it was a good idea for this to be the case for arbitrary attributes. Maybe special casing some attributes (such as [@@deprecated]) would be a better idea.

Personally, I don't think this "cascading" is the right solution to this problem. The underlying cause of the problem is that we are treating the attributes of bindings as semantic attributes on variables. What we should be doing is having a deprecated field on value descriptions, and setting it based on the attributes of bindings. If you do it this way then your "cascading" logic has nothing to do with attributes -- it is just a additional rule about when you set the deprecated field.

nojb · 2018-06-07T16:47:02Z

Personally, I don't think this "cascading" is the right solution to this problem. The underlying cause of the problem is that we are treating the attributes of bindings as semantic attributes on variables. What we should be doing is having a deprecated field on value descriptions, and setting it based on the attributes of bindings. If you do it this way then your "cascading" logic has nothing to do with attributes -- it is just a additional rule about when you set the deprecated field.

Adding a field to value descriptions sounds like the good solution. I will give it a shot. Thanks!

Use record type for Typecore.pattern_variable

f3d19a4

trefis approved these changes Jun 7, 2018

View reviewed changes

trefis reviewed Jun 7, 2018

View reviewed changes

nojb added 4 commits June 7, 2018 16:11

Keep track of attributes in pattern variables

3ba6d35

Add test

3f09756

Changes

d0a2bbc

Remove pv_name field

0d6569f

nojb force-pushed the pattern_variables_attributes branch from c30cad3 to cdc4c6a Compare June 7, 2018 14:13

Merge pattern matching cases

9ef3d3f

nojb force-pushed the pattern_variables_attributes branch from cdc4c6a to 9ef3d3f Compare June 7, 2018 14:19

Update Changes

14eafcd

nojb merged commit d4bd388 into ocaml:trunk Jun 7, 2018

lpw25 mentioned this pull request Jul 17, 2018

Generalize the "deprecated" alert #1804

Merged

5 tasks

vicuna mentioned this pull request Mar 14, 2019

Deprecated annotation on let-binding does not trigger #7719

Closed

Conversation

nojb commented Jun 7, 2018

Uh oh!

trefis left a comment

Choose a reason for hiding this comment

Uh oh!

trefis Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

nojb Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

trefis Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

xclerc commented Jun 7, 2018

Uh oh!

nojb commented Jun 7, 2018

Uh oh!

nojb commented Jun 7, 2018

Uh oh!

Drup commented Jun 7, 2018

Uh oh!

gasche commented Jun 7, 2018

Uh oh!

trefis commented Jun 7, 2018

Uh oh!

trefis commented Jun 7, 2018

Uh oh!

gasche commented Jun 7, 2018

Uh oh!

nojb commented Jun 7, 2018

Uh oh!

alainfrisch commented Jun 7, 2018

Uh oh!

lpw25 commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb commented Jun 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lpw25 commented Jun 7, 2018 •

edited

Loading