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

Fix parse panic #1071

Merged
merged 5 commits into from
Feb 17, 2021
Merged

Fix parse panic #1071

merged 5 commits into from
Feb 17, 2021

Conversation

robdockins
Copy link
Contributor

Fix parsing/typechecking issues arising from explicit type applications occurring in unexpected places.

This change restricts some programs that were previously accepted, but were rather unusual, I think. E.g.

f`{5}`{Integer}

Is a program that used to be accepted with the same meaning as

f`{5,Integer}

After this patch, the first program with an error indicating that type applications are only allowed on identifiers.

@robdockins robdockins requested a review from yav February 10, 2021 23:05
@brianhuffman
Copy link
Contributor

Are empty type applications like f`{} still allowed?

@robdockins
Copy link
Contributor Author

robdockins commented Feb 11, 2021

Yes, those are still allowed. We could rule them out, I suppose.

I think it would just be a matter of removing this line from the parser

| '{' '}' { at ($1,$2) (TTyApp []) }

Or, we could do it at a later stage and maybe get better error messages.

Copy link
Member

@yav yav left a comment

Choose a reason for hiding this comment

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

Looks good to me, just maybe add the example, and I am not sure about the paren case, but I am also not sure if it actually matters.

@brianhuffman
Copy link
Contributor

If you want to apply a type to an infix operator, you definitely will need parens, e.g. (+)`{Integer} I suppose that syntax might not use the same "paren" parse AST constructor, but it seems consistent to allow the parens for non-infix identifiers too.

These are all related to the parsing of explicit type application
forms, and the fixup pass that is supposed to attach them to
function identifiers.
to a kind-checking error.  This at least provides actionable
feedback about where the error occurs.

We should also improve the parser post-processing pass to
handle these situations better, but this at least removes
the panic.
We now reject situations where the user writes a type application
on a term that is not an identifier, and we correctly unwind
and reapply tuple and field selectors.

Fixes #962
Fixes #1045
Fixes #1050
@robdockins
Copy link
Contributor Author

Yeah, I don't see a compelling reason to rule out things like (name)`{a = Integer}, so I guess we might as well leave it.

@yav
Copy link
Member

yav commented Feb 17, 2021

@brianhuffman I don't think that (+) would result in Parens in the AST, I believe it should just make a different kind of name.

I was confused by the parens because I think of type application as a property of a name rather then expression, because in the current system only named thing can be polymorphic.

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.

3 participants