Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support accessing associated functions by member access into facets #4872

Merged
merged 25 commits into from
Feb 4, 2025

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Jan 30, 2025

For an expression such as (Type as Interface).AssocFn(), track the Self type Type in the result of the member access so that it's available when checking the function call.

This introduces a new kind of type, ImplFunctionType, that represents the type of a function that is expected within an impl, modeled as the type of the function within the interface plus a value to use as Self. Calls to values of this type behave like calls to the underlying function except that the Self parameter is pre-bound to the self type from the facet.

In order to support this, fix an issue where the imported list of generic bindings lost their association with their enclosing generic. This adds a little complexity to import_ref, including a new recursive cycle that I intend to address in a follow-up PR.

…fic.

This looks like it doesn't work out quite right: we want to distinguish
between a function in an interface and a function in a particular impl,
so giving them different types seems valuable.
Don't attempt to deduce it from the call arguments.
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.

Just noting to be sure what I'm supposed to only lightly review: I'm assuming the LoadImportRef loop is the recursive cycle you're referring to in the PR description.

toolchain/check/generic.h Outdated Show resolved Hide resolved
toolchain/check/import_ref.cpp Outdated Show resolved Hide resolved
toolchain/check/deduce.cpp Outdated Show resolved Hide resolved
toolchain/sem_ir/typed_insts.h Outdated Show resolved Hide resolved
toolchain/sem_ir/function.cpp Show resolved Hide resolved
toolchain/check/member_access.cpp Outdated Show resolved Hide resolved
toolchain/check/interface.cpp Outdated Show resolved Hide resolved
toolchain/check/interface.cpp Show resolved Hide resolved
toolchain/check/interface.cpp Outdated Show resolved Hide resolved
toolchain/check/interface.cpp Outdated Show resolved Hide resolved
zygoloid and others added 5 commits January 31, 2025 23:16
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
@zygoloid zygoloid requested a review from jonmeow February 1, 2025 01:48
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.

Generally LG, but note I think the formatting is a bug I missed in the first pass. I think it'll be straightforward to fix if I'm reading it correctly; feel free to merge after.

toolchain/sem_ir/typed_insts.h Outdated Show resolved Hide resolved
sem_ir.insts().GetAs<FunctionType>(inst.interface_function_type_id);
auto fn_name_id = sem_ir.functions().Get(fn_inst.function_id).name_id;
out << "<type of " << sem_ir.names().GetFormatted(fn_name_id) << " in ";
case CARBON_KIND(FunctionTypeWithSelfType inst): {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed this on my first pass. Is this tested somewhere?

It looks like this will format as <type of in self_id>interface_function_type_id -- shouldn't fn_inst be examined last?. Looking through tests (git grep "type of" **/*.carbon), I see <type of F> in check/testdata/operators/builtin/fail_redundant_compound_access.carbon as the only <type of that's tested. Not sure how easy this is to test, but if you have ideas it might be good to add something (also for the surrounding changes, since I think only FunctionType is tested, and nothing qualified).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed and tested in toolchain/check/testdata/interface/no_prelude/fail_assoc_fn_invalid_use.carbon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor and maybe not specific to this PR, but <type of F in T as I> might be clearer as <type of F in (T as I)> to avoid confusion about <type of (F in T) as I>. But I don't know if the as is always there, so maybe not a clear fix... I mean, <type of F in (X)> would be not-so-good, and the current format is probably usually clear enough.

@zygoloid zygoloid enabled auto-merge February 4, 2025 01:26
@jonmeow
Copy link
Contributor

jonmeow commented Feb 4, 2025

LG

@zygoloid zygoloid added this pull request to the merge queue Feb 4, 2025
Merged via the queue into carbon-language:trunk with commit fcfb134 Feb 4, 2025
8 checks passed
@zygoloid zygoloid deleted the toolchain-facet-access branch February 4, 2025 23:40
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants