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

Support for templated impl declarations #2700

Merged
merged 3 commits into from
Mar 22, 2023

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Mar 20, 2023

The strategy that we use for now to support template instantiation is to check the impl declaration as if it were a generic, but to defer all checking of the impl definition until we see a use in which all template parameters have arguments. At that point, we clone the impl definition and type-check the whole thing, with constant values set on the template parameters corresponding to the given arguments.

No caching of template instantiations is performed yet; each time we form a reference to a template instantiation, we instantiate it afresh. We also don't implement the name lookup rule from #949 yet; lookups during template instantiation look only in the actual type and not in the constraint.

Depends on #2699

@github-actions github-actions bot added dependent Depends on another issue/PR explorer Action items related to Carbon explorer code and removed dependent Depends on another issue/PR labels Mar 20, 2023
@github-actions
Copy link

This PR/issue depends on:

The strategy that we use for now to support template instantiation is to
check the impl declaration as if it were a generic, but to defer all
checking of the impl definition until we see a use in which all template
parameters have arguments. At that point, we clone the impl definition
and type-check the whole thing, with constant values set on the template
parameters corresponding to the given arguments.

No caching of template instantiations is performed yet; each time we
form a reference to a template instantiation, we instantiate it afresh.
@zygoloid zygoloid force-pushed the explorer-template branch from 3b5816c to 82679b4 Compare March 22, 2023 19:06
@zygoloid zygoloid marked this pull request as ready for review March 22, 2023 19:06
@zygoloid zygoloid requested a review from jonmeow March 22, 2023 19:06
This currently doesn't work properly.
}

impl forall [template T:! type] T as GetX {
// CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/template/fail_no_member.carbon:[[@LINE+1]]: member access, unexpected i32 in self.x
Copy link
Contributor

Choose a reason for hiding this comment

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

This error is kind of confusing; where in self.x is the unexpected i32? This looks like you're falling through to the "unhandled" case of TypeCheckExp, not a deliberately written error message.

Copy link
Contributor

@jonmeow jonmeow Mar 22, 2023

Choose a reason for hiding this comment

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

i.e., handling this should probably be part of this PR, since it seems you may be adding code that deliberately reaches an unhandled code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't new in this PR: https://carbon.godbolt.org/z/f87nc4e4f

(It would be nice to include a note saying that we're performing an instantiation with T=i32; there's a TODO in the code for that already.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this would make sense for a "good first issue"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, filed as #2705.

explorer/interpreter/type_checker.cpp Show resolved Hide resolved
explorer/interpreter/type_checker.cpp Outdated Show resolved Hide resolved
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
@zygoloid zygoloid merged commit e0c9076 into carbon-language:trunk Mar 22, 2023
jonmeow added a commit to jonmeow/carbon-lang that referenced this pull request Mar 29, 2023
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