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

Do not discard attributes attached to pattern variables #1822

Merged
merged 7 commits into from Jun 7, 2018

Conversation

Projects
None yet
7 participants
@nojb
Copy link
Contributor

commented Jun 7, 2018

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

trefis approved these changes Jun 7, 2018

Copy link
Contributor

left a comment

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.

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

This comment has been minimized.

Copy link
@trefis

trefis Jun 7, 2018

Contributor

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 comment has been minimized.

Copy link
@nojb

nojb Jun 7, 2018

Author Contributor

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

| [],(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, [])))

This comment has been minimized.

Copy link
@trefis

trefis Jun 7, 2018

Contributor

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

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2018

  • 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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

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

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

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.

@nojb nojb force-pushed the nojb:pattern_variables_attributes branch from c30cad3 to cdc4c6a Jun 7, 2018

@nojb nojb force-pushed the nojb:pattern_variables_attributes branch from cdc4c6a to 9ef3d3f Jun 7, 2018

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

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:

  1. @Drup's proposal: warn on attributes which are not in the right branch
  2. 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

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

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

@trefis

This comment has been minimized.

Copy link
Contributor

commented on bytecomp/translclass.ml in 0d6569f Jun 7, 2018

That looks fishy. Once again, looking at the uses I think we should be able to get rid of this and cleanup some more.
But let's not pollute this GPR further: I'll have a look at cleaning this once your PR is merged.

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2018

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!

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

2 checks passed

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

@lpw25 lpw25 referenced this pull request Jul 17, 2018

Merged

Generalize the "deprecated" alert #1804

2 of 5 tasks complete
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.