Skip to content

ppxs: no empty polymorphic type binders in parsetree#11148

Merged
Octachron merged 1 commit intoocaml:trunkfrom
Octachron:empty_ptyp_poly
Feb 9, 2024
Merged

ppxs: no empty polymorphic type binders in parsetree#11148
Octachron merged 1 commit intoocaml:trunkfrom
Octachron:empty_ptyp_poly

Conversation

@Octachron
Copy link
Member

This PR proposes to check that ppxs do not produce parsetree with empty explicit universal quantifications, for instance in fantasy syntax

type t = { r: . int -> int }

instead of

type t = { r: 'a 'b. int -> int }

Adding this invariant avoids the need to handle unexpected and sometimes invisible (see #11129) Ptyp_poly([],_) in the typechecker itself.

There is however a small exception to this rule for constraint on patterns, due to the fact that

let p : t = e

is currently encoded as

let (p: . t) = (e:t)

and the ast pretty printer relies on this encoding to resugar the second form in the first form.

close #11129

@gasche
Copy link
Member

gasche commented Mar 31, 2022

The fact that our own parsetree uses this empty-polymorhpism gives me cold feet. We are telling PPX authors to that it is invalid, with a special case for when our own parsetree producer actually uses it. Is there a reasonable way to avoid using this horrible hack in the parsetree, to avoid the hypocrisy?

@gasche
Copy link
Member

gasche commented Mar 31, 2022

For example, maybe we could:

  • enrich Parsetree.value_binding with a toplevel pvb_constraint field that stores the handled-in-a-special-way toplevel annotation
  • or, if we want to stay in the country of ugly workarounds, add a magic ocaml-namespaced attribute to notify that the toplevel constraint is to be desugared specially

@garrigue
Copy link
Contributor

Well, I suppose that the special processing for non-quantified types could be done in typecore.ml rather than in parser.mly. Just have to be careful of what is assumed where.

@Octachron
Copy link
Member Author

I had a look at moving the processing of constraints on let binding to typecore. That seems potentially simpler, I will submit a PR once my todo list for 5.0 is less full.

@gasche
Copy link
Member

gasche commented Jan 13, 2023

ping @Octachron

@Octachron
Copy link
Member Author

I have updated this PR now that the parsetree is not using empty binders in encoding anymore.

@smuenzel
Copy link
Contributor

small side-question about these kinds of parsetree invariants: What about introducing a non-empty list type to prevent something like this by construction? maybe that's more useful for records/tuples -- but preventing invalid data by proper type construction seems like a good property.

@Octachron
Copy link
Member Author

It would make sense, but it is a more involved change:
First, it is a visible change in the parsetree.
Second, once we have a non-empty list, we probably want a non-empty list module that mirrors the List module.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

Sorry for forgetting about this PR. I am fine with the new, simpler version. This might break some ppx-produced code, but that code would probably have broken in more obscure ways later on in the type-checker, so this is in fact an interface improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Conflicting error message whether to be or not to be a function

4 participants