Skip to content

Lower the level of ty_res rather than funct#11519

Open
garrigue wants to merge 1 commit intoocaml:trunkfrom
COCTI:fix-ty_res-level
Open

Lower the level of ty_res rather than funct#11519
garrigue wants to merge 1 commit intoocaml:trunkfrom
COCTI:fix-ty_res-level

Conversation

@garrigue
Copy link
Copy Markdown
Contributor

@garrigue garrigue commented Aug 30, 2022

Some old code was lowering the level on the wrong type.
I.e., when we return a typedtree node, we should ensure that its type is at current_level, but we don't need to ensure that on children.

This was probably a bug, but it was not detected by the testsuite.

@garrigue
Copy link
Copy Markdown
Contributor Author

Actually, there is no need to lower the level here, as this is already done by the call to rue.
However, there is no need to lower the level in funct either, so just remove the line.

@gasche
Copy link
Copy Markdown
Member

gasche commented Aug 30, 2022

In principal mode, the code type_sfunct above would call generalize_structure on funct.exp_type, which puts it at generic level, right? Could it be that the line you removed is meant to un-do that generalization, and should stay?

Looking at the git history, relevant commits are 0a82360, which introduced the generalization of func.exp_type (with a matching instance call at the end of application typing) and 0edba97, which turned the instance call into the manual lowering there is in trunk.

@garrigue
Copy link
Copy Markdown
Contributor Author

Yes, we already checked the history, but I'm still not clear why this would be needed.
It is only useful if the original type of funct.exp_type escapes in the type of the application, but my intuition would be that we only use instances of it when typing the application. I will check again, but this looks like a spurious step.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants