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

Call remove_pos only where a type comes into the system #375

Merged
merged 3 commits into from
Feb 2, 2022

Conversation

zuiderkwast
Copy link
Collaborator

Remove_pos normalizes the syntactical representation of a type, by recursively removing the location annotation (line and column). This is to be able to compare types using equality and pattern matching in the type checker.

This should only be done once on each type and only when the type comes into the type checker, i.e. when types are extracted from a parse tree.

This PR sorts out the mess, makes sure it's done where it should and removes it where it shouldn't be done.

Note that gradualizer_db doesn't use this, so we call remove_pos on the types returned by gradualizer_db.

@zuiderkwast zuiderkwast requested a review from erszcz February 1, 2022 17:05
@erszcz
Copy link
Collaborator

erszcz commented Feb 1, 2022

This looks good, but I think there are a few places where remove_pos is now redundant:

Apart from the above, there are a few cases where it's called on a value returned from various gradualizer_db functions - for example:

normalize_rec(typelib:remove_pos(T), Env, Unfolded);

Is there a reason to keep types with their position in the DB or could we also remove it there, to remove the redundancy at call sites?

@zuiderkwast
Copy link
Collaborator Author

I think there are a few places where remove_pos is now redundant

Thanks, the tests still pass when I remove these.

Is there a reason to keep types with their position in the DB or could we also remove it there, to remove the redundancy at call sites?

The reason I didn't do it already is that this is no mess so far. Remove_pos was simply not done there, without exceptions. The mess was only in typechecker. But since gradualizer_db does other stuff such as absform:normalize_function_type_list/1 and absform:normalize_record_field/1, I think it can do remove_pos too. It will make everything simpler. 👍 I'll do it tomorrow.

Copy link
Collaborator

@erszcz erszcz left a comment

Choose a reason for hiding this comment

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

That's neat!

@zuiderkwast zuiderkwast merged commit 87bb404 into josefs:master Feb 2, 2022
@zuiderkwast zuiderkwast deleted the remove_pos branch February 4, 2022 13:34
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.

2 participants