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

Unsharing pattern types #1909

Merged
merged 3 commits into from Jul 26, 2018

Conversation

Projects
None yet
3 participants
@trefis
Copy link
Contributor

commented Jul 16, 2018

Extracts the least controversial part #1675 .

@trefis trefis force-pushed the trefis:unsharing-pattern-types branch from 0a2d93b to d7edc97 Jul 17, 2018

@garrigue
Copy link
Contributor

left a comment

Globally, this looks correct. However, I'm wondering what should be returned in pat_type. We seem to have an invariant to keep it non-generic, but wouldn't it make sense to just always put the (generalized) expected_ty, from a semantical point of view?

@@ -1085,13 +1085,18 @@ and type_pat_aux ~exception_allowed ~constrs ~labels ~no_existentials ~mode
~env
in
let loc = sp.ppat_loc in
let rup k x =
if constrs = None then (ignore (rp x));

This comment has been minimized.

Copy link
@garrigue

garrigue Jul 19, 2018

Contributor

You record the type of the pattern before unification with the expected type.
This is a change w.r.t. the previous behavior.
Is this intentional?

This comment has been minimized.

Copy link
@garrigue

garrigue Jul 19, 2018

Contributor

Sorry, looking at what does Stypes.record, you are just registering the node earlier, and unification can still occur afterwards. So one has more information if the next unification fails, and the same otherwise. Fine.

begin_def ();
let expected_ty = instance expected_ty in
end_def ();
generalize_structure expected_ty;

This comment has been minimized.

Copy link
@garrigue

garrigue Jul 19, 2018

Contributor

Here it should be OK to use generalize: since we are matching a value, different members could be seen as using independent instances.

This comment has been minimized.

Copy link
@trefis

trefis Jul 24, 2018

Author Contributor

I don't think it makes a difference here: expected_ty doesn't contain any type variable at the generic level when we receive it. Which means that after the

begin_def;
instance;
end_def;

sequence, there won't be any generalizable variables.

Given that, I'd rather keep the call to generalize_structure here, for uniformity.

This comment has been minimized.

Copy link
@garrigue

garrigue Jul 25, 2018

Contributor

Oh, I see. I hadn't seen that due to the generalize_structure in type_cases, there are no generalized type variables in the type.
This would change if switch later to a more agressive policy of taking full instances, that could be fully generalized soundly.

But then it seems that the only improvement of the current patch is to avoid some warnings with label/constructor disambiguation. Is there any other practical change?

end_def ();
generalize_structure expected_ty;
generalize_structure ty_res;
List.iter generalize_structure ty_args;

This comment has been minimized.

Copy link
@garrigue

garrigue Jul 19, 2018

Contributor

Here the sharing of type variables is needed, so this indeed has to be generalize_structure.

pat_desc = Tpat_record (lbl_pat_list, closed);
pat_loc = loc; pat_extra=[];
pat_type = expected_ty;
pat_type = record_ty;
pat_attributes = sp.ppat_attributes;

This comment has been minimized.

Copy link
@garrigue

garrigue Jul 19, 2018

Contributor

Shouldn't it be instance record_ty ? And why call rup rather than rp (record_ty is already an instance of expected_ty) ?

This comment has been minimized.

Copy link
@trefis

trefis Jul 24, 2018

Author Contributor

It should be instance record_ty you are correct.
However record_ty is not necessarily an instance of expected_ty, in fact in most cases it won't be: it will be only when extract_concrete_record succeeds.

This comment has been minimized.

Copy link
@trefis

trefis Jul 24, 2018

Author Contributor

Note: I pushed a commit adding the instance taking.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

Thanks for the review. I haven't had time to think much about the question you ask (or to fix the small issues in the code) yet, but I'll get back to you some time next week.

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

I'm copying here an inline comment, as it might no be very visible:

Since, in type_cases, generalize_structure is called before calling type_pattern, expected_ty cannot contain generalized type variables.

As a result, it seems that the only improvement of the current patch is to avoid some warnings with label/constructor disambiguation. Is there any other practical change?

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

I haven't actually tested it with this version of the patch, but I think it should address ambiguity errors like this one:

# type 'a r = { a : 'a; b: 'a };;
type 'a r = { a : 'a; b : 'a; }

# type 'a ty = Int : int ty | Float : float ty;;
type 'a ty = Int : int ty | Float : float ty

# let foo (type a) (ty : a ty) (x : a r) =
    match ty, x with
    | Int, { a = 3; b } -> b
    | _ -> assert false;;
@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

I haven't actually tested it with this version of the patch, but I think it should address ambiguity errors like this one: [...]

Indeed, that's the test I add with my first commit: at that time it still complains that the type of b is ambiguous, but this is then fixed by the later commits.

As a result, it seems that the only improvement of the current patch is to avoid some warnings with label/constructor disambiguation. Is there any other practical change?

The practical change is illustrated in the test suite by the example Leo just repeated.
As for the principality warnings regarding disambiguation: this PR doesn't contain changes to that effect. I wasn't sure whether we'd completely agreed on that point when we decided to split into smaller PRs. So I went with this small PR first, as it seemed to be the least controversial :)

I plan to open a new PR containing (only) the changes relating to principality warnings as a follow up to this PR.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

However, I'm wondering what should be returned in pat_type. We seem to have an invariant to keep it non-generic, but wouldn't it make sense to just always put the (generalized) expected_ty, from a semantical point of view?

Perhaps. Although I think putting the instance makes just as much sense?
Also, in the implementation, in some cases we don't have a generic type at hand after typing the pattern, which I take as a hint that putting the non-generic type is the right thing to do.

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

My comment concerning whether pat_type should be generic is also for the future, if we propagate fully generic types: logically this type should be understood as the input of the pattern.

For now I'm satisfied with the current PR, which keeps all invariants, and so is safe.

@trefis trefis force-pushed the trefis:unsharing-pattern-types branch from 4a3c5f3 to f658de3 Jul 25, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

Understood.

Thanks for looking at this!

I've now rebased and squashed the commits, I'll merge once the CIs are happy (hopefully appveyor won't time out this time).

@trefis trefis force-pushed the trefis:unsharing-pattern-types branch from f658de3 to d931b9c Jul 25, 2018

@trefis trefis merged commit 9c53eae into ocaml:trunk Jul 26, 2018

2 checks passed

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

@trefis trefis deleted the trefis:unsharing-pattern-types branch Jul 26, 2018

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.