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

Split TypeCheckExp into recursive and non-recursive steps #1277

Closed
wants to merge 3 commits into from
Closed

Split TypeCheckExp into recursive and non-recursive steps #1277

wants to merge 3 commits into from

Conversation

zygoloid
Copy link
Contributor

This splits expression type-checking into two parts:

  • TypeCheckExpOperands type-checks the operands of an expression recursively, but does not type-check the expression itself.
  • TypeCheckOneExp type-checks an expression, assuming its operands have already been type-checked.

The existing TypeCheckExp function then calls TypeCheckExpOperands followed by TypeCheckOneExp.

This change has two main motivations:

  1. Support creation and type-checking of new expressions created by desugaring, with operands that have already been type-checked. For example, if we determine that the expression F(e) requires e to be implicitly converted to i32, we may rewrite it as F(e.(ImplicitAs(i32).Convert)()), but we don't want to type-check e a second time after this rewriting. See Support user-defined implicit conversions via ImplicitAs #1273 for an example of how this is used.
  2. Systematically avoid bugs where type-checking an expression can look at properties of its subexpressions before those properties are computed.

@zygoloid zygoloid requested a review from a team as a code owner May 18, 2022 23:56
@zygoloid zygoloid added the explorer Action items related to Carbon explorer code label May 19, 2022
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

I think I find the change here a bit odd versus what's been discussed on #1256, noting that this seems to be in direct conflict with trying to pair TypeCheckExp/InterpExp invocations.

Part of why I think this is odd is that it's turning expressions into a special-case, and I'm not sure that's necessary: e.g., pattern checking still follows the prior flow. But even if you split out that two, then it would seem a little odd (as it does here) because those functions aren't mutually calling each other. Thus this feels like it may end up forcing divergent solutions to the same problem, ensuring that expressions are being type checked.

Regarding #1273, we've discussed adding something like has_static_type to determine whether an AST node has been type checked: what would you think of using that instead in #1273 to avoid duplicate type checking?

@zygoloid
Copy link
Contributor Author

I think I find the change here a bit odd versus what's been discussed on #1256, noting that this seems to be in direct conflict with trying to pair TypeCheckExp/InterpExp invocations.

I think we can follow both this direction and that of #1256, by having the checking performed by TypeCheckExpOperands call TypeCheckTypeOperand when appropriate instead of TypeCheckExp for type subexpressions. But as I mentioned on discord, the direction in #1256 seemed premature to me because it reflects a bunch of calls that we happen to make together but that we might want to separate in future; this change is such a future.

Part of why I think this is odd is that it's turning expressions into a special-case, and I'm not sure that's necessary: e.g., pattern checking still follows the prior flow.

Hm. Pattern type checking currently has a quite strange flow where the type checking depends on an expected destination type. Per this discussion we may want to move away from that model, but until we do, I think that performing this same kind of split on pattern type checking would be more awkward.

Regarding #1273, we've discussed adding something like has_static_type to determine whether an AST node has been type checked: what would you think of using that instead in #1273 to avoid duplicate type checking?

I think that would work. It doesn't seem as conceptually clean as this approach to me, though.

@zygoloid
Copy link
Contributor Author

Abandoned in favor of @jonmeow's suggestion to add something like has_static_type.

@zygoloid zygoloid closed this May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
explorer Action items related to Carbon explorer code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants