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

Take into account position of main argument of a projection when inserting implicit arguments for syntax t.(f) #14606

Conversation

herbelin
Copy link
Member

@herbelin herbelin commented Jul 5, 2021

Kind: fix and enhancement

Depends on mattam82/Coq-Equations#413 (or at worst on tolerant Loc.merge in #14618).

Fixes #4167 and improve error message when a field has a wrong number of explicit parameters

This is done by introducing special functions for interning/externing which cut the implicit arguments signature of a projection into a parameter part and an extra functional part (if ever the projection happens to return a function with its own implicit arguments).

  • Added / updated test-suite
  • Entry added in the changelog

@herbelin herbelin added kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. part: elaboration The elaboration engine, also known as the pretyper. part: implicit arguments The implicit arguments mechanism, generalizable variables, etc. labels Jul 5, 2021
@herbelin herbelin added this to the 8.15+rc1 milestone Jul 5, 2021
@herbelin herbelin force-pushed the master+improve-error-wrong-number-explicit-field-parameter branch from db7b031 to b711386 Compare July 5, 2021 19:52
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 5, 2021

Hey, I have detected that there were CI failures at commit b711386 without any failure in the test-suite.
I checked that the corresponding jobs for the base commit 8560c27 succeeded. You can ask me to try to extract a minimal test case from this so that it can be added to the test-suite.
If you tag me saying @coqbot ci minimize, I will minimize the following target: ci-argosy.

2 similar comments
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 5, 2021

Hey, I have detected that there were CI failures at commit b711386 without any failure in the test-suite.
I checked that the corresponding jobs for the base commit 8560c27 succeeded. You can ask me to try to extract a minimal test case from this so that it can be added to the test-suite.
If you tag me saying @coqbot ci minimize, I will minimize the following target: ci-argosy.

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 5, 2021

Hey, I have detected that there were CI failures at commit b711386 without any failure in the test-suite.
I checked that the corresponding jobs for the base commit 8560c27 succeeded. You can ask me to try to extract a minimal test case from this so that it can be added to the test-suite.
If you tag me saying @coqbot ci minimize, I will minimize the following target: ci-argosy.

@herbelin herbelin force-pushed the master+improve-error-wrong-number-explicit-field-parameter branch from b711386 to 5b9df34 Compare July 8, 2021 08:02
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 8, 2021

Hey, I have detected that there were CI failures at commit 5b9df34 without any failure in the test-suite.
I checked that the corresponding jobs for the base commit 444aa95 succeeded. You can ask me to try to extract a minimal test case from this so that it can be added to the test-suite.
If you tag me saying @coqbot ci minimize, I will minimize the following target: ci-metacoq.

@herbelin herbelin force-pushed the master+improve-error-wrong-number-explicit-field-parameter branch from 5b9df34 to 1397502 Compare July 8, 2021 20:04
@herbelin herbelin added the needs: merge of dependency This PR depends on another PR being merged first. label Jul 8, 2021
@herbelin herbelin force-pushed the master+improve-error-wrong-number-explicit-field-parameter branch 2 times, most recently from 91ff86c to 20a865e Compare July 8, 2021 21:11
@herbelin herbelin marked this pull request as ready for review July 9, 2021 05:38
@herbelin herbelin requested review from a team as code owners July 9, 2021 05:38
@herbelin herbelin added the kind: fix This fixes a bug or incorrect documentation. label Jul 10, 2021
@herbelin herbelin changed the title Improve error message when a field has a wrong number of explicit parameter in the x.(f) syntax Take into account position of main argument of a projection when inserting implicit arguments for syntax t.(f): fixes #4167 and improve error message when a field has a wrong number of explicit parameters Jul 10, 2021
herbelin added a commit to herbelin/github-coq that referenced this pull request Jul 10, 2021
@herbelin herbelin force-pushed the master+improve-error-wrong-number-explicit-field-parameter branch from 20a865e to ed30ef4 Compare July 10, 2021 06:00
if expl then
[], []
else
let ngivenparams = List.length (List.filter (fun (_,x) -> x == None) args1) in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use List.count and Option.is_empty.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

let args2 = intern_impargs p env imps2 subscopes2 args2 in
smart_gapp p loc (args0@args1@c::args2)
| None ->
(* Tolerate a use of t.(f) notation for an ordinary application until a decision is taken about it *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is making the t.(p) notation straying further than the application it used to stand for. Maybe we should take the decision soon. Note that the comment above about "officiality" is wrong, IIUC the code still takes the projection path when the head is an arbitrary reference.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is making the t.(p) notation straying further than the application it used to stand for.

Yes and no. In this PR, it is still an application but with a treatment of implicit arguments which takes into account that the main argument splits the implicit arguments into two parts.

So, unless I'm missing something. nothing is changed wrt references which have no implicit arguments.

Note that the comment above about "officiality" is wrong

Would replacing "An official projection" by "A reference registered as projection" be ok?

IIUC the code still takes the projection path when the head is an arbitrary reference.

If by projection path you mean intern_proj, yes, since intern_proj is called whenever the t.(f) syntax is used. Is that ok or do you expect something else? For instance, would you prefer that another terminology than "proj" is used to talk about the t.(f) notation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ppedrot: I used "A reference registered as projection", hoping it is compatible with your view. Should the names of functions (like intern_proj, etc.) be reconsidered further to avoid any ambiguity? Did I address your questions?

In any case, dependency is merged, will you assign?

[bedrock2's failure is waiting about mit-plv/bedrock2#190]


let select_impargs_size_for_proj ~nexpectedparams ~ngivenparams ~nextraargs impls =
let split_implicit_params imps =
if imps = [] then (nexpectedparams, [], []) else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use List.is_empty.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Granting your wish.

herbelin added a commit to herbelin/github-coq that referenced this pull request Jul 10, 2021
@herbelin herbelin force-pushed the master+improve-error-wrong-number-explicit-field-parameter branch from ed30ef4 to b0d1791 Compare July 10, 2021 08:58
herbelin added a commit to herbelin/github-coq that referenced this pull request Jul 11, 2021
@herbelin herbelin force-pushed the master+improve-error-wrong-number-explicit-field-parameter branch from b0d1791 to ca6221e Compare July 11, 2021 09:50
herbelin added a commit to herbelin/github-coq that referenced this pull request Jul 14, 2021
@herbelin herbelin force-pushed the master+improve-error-wrong-number-explicit-field-parameter branch from ca6221e to 4799139 Compare July 14, 2021 08:17
@herbelin herbelin force-pushed the master+improve-error-wrong-number-explicit-field-parameter branch from 4799139 to 84bf06c Compare July 18, 2021 16:58
@ppedrot ppedrot merged commit 07bc03e into coq:master Jul 19, 2021
@RalfJung
Copy link
Contributor

Is it possible that this is the cause of #14703?

@RalfJung
Copy link
Contributor

RalfJung commented Jul 23, 2021

It looks like this PR also changed notation parsing:
(Σ[ ty; rc ty ]).(ty_size) now fails ("unknown interpretation for notation") where ty_size (Σ[ ty; rc ty ]) still works; here Σ[...] is a notation in a scope that is not opened globally but via Arguments ty_size.

@herbelin
Copy link
Member Author

I could not reproduce. Could you give an example if possible?

@SkySkimmer SkySkimmer changed the title Take into account position of main argument of a projection when inserting implicit arguments for syntax t.(f): fixes #4167 and improve error message when a field has a wrong number of explicit parameters Take into account position of main argument of a projection when inserting implicit arguments for syntax t.(f) Jul 23, 2021
@herbelin
Copy link
Member Author

For instance, the following works:

Declare Scope foo_scope.
Delimit Scope foo_scope with foo.
Record prod A B := pair { fst : A ; snd : B }.
Notation "[[ t , u ]]" := (pair _ _ t u) : foo_scope.
Arguments fst {A B} p%foo.
Check [[2,3]].(fst).

@RalfJung : How different is your real example?

@RalfJung
Copy link
Contributor

This is the change that was required. I can try to minimize this.

@RalfJung
Copy link
Contributor

Hm, my attempt to isolate this also failed:

From Coq Require Import List.
Export ListNotations.

Record type :=
  { ty_size : nat;
    ty_own : list nat -> Prop;
    ty_size_eq vl : ty_own vl -> length vl = ty_size;
  }.

Declare Scope lrust_type_scope.
Delimit Scope lrust_type_scope with T.
Bind Scope lrust_type_scope with type.

Axiom sum : list type -> type.

Notation "Σ[ ty1 ; .. ; tyn ]" :=
  (sum (cons ty1%T (..(cons tyn%T nil)..))) : lrust_type_scope.

Axiom t : type.
Check (ty_size Σ[t; t]).
Check ((Σ[t; t]).(ty_size)).

Something must still be different than in the full lambda-rust, but I have no idea what.

@herbelin
Copy link
Member Author

herbelin commented Jul 23, 2021

Actually, this has nothing to do with scopes and notations. It is another instance of the class argument of ty_size being forced to be explicit and in position of main argument when using the .( ) notation! No more worries than #14703!

@RalfJung
Copy link
Contributor

RalfJung commented Jul 23, 2021

I don't see that. Σ[ ty; rc ty ] is of type type and ty_size is a primitive projection of type.

Using foo.(ty_size) works in many other places, it just does not work here. And the error explicitly talks about "unknown interpretation for notation". I am pretty sure this is a different issue than #14703.

@RalfJung
Copy link
Contributor

For example, further up the same file we do new [ #(option ty).(ty_size) ] and that works just fine. That should require exactly the same kind of inference as new [ #(Σ[ ty; rc ty ]).(ty_size) ].

@herbelin
Copy link
Member Author

You're right, there is a bug (I did not test on the right branch apparently). I'm going to submit a fix soon.

@RalfJung
Copy link
Contributor

Awesome, thanks. :)

herbelin added a commit to herbelin/github-coq that referenced this pull request Jul 26, 2021
herbelin added a commit to herbelin/github-coq that referenced this pull request Jul 30, 2021
ppedrot added a commit that referenced this pull request Jul 31, 2021
jfehrle pushed a commit to jfehrle/coq that referenced this pull request Sep 21, 2021
jfehrle pushed a commit to jfehrle/coq that referenced this pull request Sep 21, 2021
@SkySkimmer
Copy link
Contributor

There's a changelog, but shouldn't there be something in the doc too?

coqbot-app bot added a commit that referenced this pull request Jan 10, 2022
…best implicit arguments signature for projections

Reviewed-by: SkySkimmer
Ack-by: artagnon
Co-authored-by: SkySkimmer <SkySkimmer@users.noreply.github.com>
SkySkimmer added a commit to SkySkimmer/coq that referenced this pull request Jan 11, 2022
…n computing best implicit arguments signature for projections
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. kind: fix This fixes a bug or incorrect documentation. part: elaboration The elaboration engine, also known as the pretyper. part: implicit arguments The implicit arguments mechanism, generalizable variables, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dot notation for class projections should be smarter about inferring instance
4 participants