Allow impls with sufficiently primitive generic parameters to be declared final. by zygoloid · Pull Request #6511 · carbon-language/carbon-lang

zygoloid · 2025-12-16T20:44:44Z

If all the generic parameters of an impl have types that mean they can't possibly be further specialized in other libraries, then allow the impl to be declared final. For example, if an impl's only generic parameter is of type i32, then it's not possible for an impl in another library to specialize it, because such an impl must necessarily be an orphan, so we can allow it to be final and check locally that it's not specialized.

We treat a generic parameter as being unable to support further specialization if its type is bool, an integer or floating-point type, or an integer, floating-point or character literal type, or if it's built exclusively from such types. Critically, if there's a facet type (including type) somewhere within the object representation of the parameter type, we assume it can be further specialized.

See #6500 for details on why this is important. This doesn't resolve that issue, but gets us incrementally closer.

…vely final. See carbon-language#6500 for details on why this is important.

Add testing.

toolchain/check/impl.cpp

danakj · 2025-12-16T22:09:14Z

toolchain/check/impl.cpp

+      // TODO: How should we handle the case where the parameter type is
+      // incomplete? Can we defer determining whether an impl is effectively
+      // final until we see its definition? It's not clear that a parameter of
+      // incomplete type can be used in an impl declaration, so this may not
+      // matter. For now we treat such impls as not effectively final.


FWIW We don't need to decide this until we build/have the full witness table, which is either the definition or the first time we wrote something into it.

danakj · 2025-12-16T22:13:47Z

toolchain/check/impl_lookup.cpp

          result.has_final_value() &&
-          !IsImplEffectivelyFinal(context,
-                                  context.impls().Get(candidate.impl_id))) {
+          context.impls().Get(candidate.impl_id).final_kind ==


Can we pass the Impl& to GetWitnessIdForImpl and avoid a second lookup here?

danakj · 2025-12-16T22:24:20Z

toolchain/check/impl.cpp

+  while (!worklist.empty()) {
+    type_id = worklist.pop_back_val();
+
+    type_id = context.types().GetObjectRepr(type_id);


Can you leave a comment explaining this? Is there a test that fails without it?

Added brief comment and some more explicit test coverage. (This was covered by the tests for eg i32, since it's an adapter for IntType, but now I'm more directly testing an adapter.)

danakj · 2025-12-16T22:27:19Z

toolchain/check/impl_lookup.cpp

  }

-  if (query_is_concrete || impl.is_final) {
-    // TODO: These final results should be cached somehow. Positive (non-None)


Did you mean to remove the TODO?

Yes, this caching was added in #6452. I could keep a TODO specifically around caching negative results if you like (we currently only cache successful lookups)?

danakj · 2025-12-16T22:31:39Z

toolchain/check/impl_lookup.cpp

-    // they can only be cached in a limited way, or the cache needs to
-    // be invalidated by writing a final impl that would match.
+  if (query_is_concrete ||
+      impl.final_kind != SemIR::FinalImplKind::Specializable) {


This does the wrong thing if:

The impl is "effectively final"

The impl is in Core prelude

The lookup is in Core prelude

It's only okay-ish because/if:

All the generic types named in TypeCanIntroduceAssociatedLibraries() are in Core prelude

We don't write any specializations in core for those generic types

The decision to be effectively final seems inherently tied to the view of the impl (different library), and can't just be a fixed property of the impl itself. I am not sure how you'd like to proceed with this PR. I tried to suggest some simple checks around being in Core but they do the wrong thing: Here, we want non-generic things to be effectively final still in Core prelude. In impl construction we don't know if the user is from the same library yet. Maybe we need to split EffectivelyFinal up to EffectivelyFinal and EffectivelyFinalOutsideCorePrelude which is at least obviously being a bit of a hack. But it feels like the way this in being stored and used needs to be changed and we might as well do that now?

Hm. I don't love the rules being different in the cross-library versus in the same-library case. This would violate the concatenation principle, among other issues.

I had a discussion with Josh about this and we came up with a different approach: we don't treat the new kind of impl as being effectively final, but instead we allow it to be explicitly declared final. We think we have a proof that this is sound.

I'm going to rework this PR in that direction.

Co-authored-by: Dana Jansens <danakj@orodu.net>

CarbonInfraBot · 2025-12-17T00:12:39Z

toolchain/check/impl.cpp

+// Determines whether the given generic parameter type can have a value that
+// introduces additional associated libraries during impl lookup.
+static auto TypeCanIntroduceAssociatedLibraries(Context& context,
+                                                SemIR::TypeId initial_type_id) -> bool {


[diff] _{reported by reviewdog 🐶}

Suggested change

SemIR::TypeId initial_type_id) -> bool {

SemIR::TypeId initial_type_id)

-> bool {

danakj

LG overall if you decide to keep pursuing this; I guess we'll need a proposal and a design change as well.

danakj · 2025-12-18T16:18:20Z

toolchain/check/impl.cpp

 #include "toolchain/check/type_completion.h"
 #include "toolchain/check/type_structure.h"
 #include "toolchain/diagnostics/diagnostic_emitter.h"
+#include "toolchain/sem_ir/builtin_function_kind.h"


danakj · 2025-12-18T16:20:21Z

toolchain/check/impl_validation.cpp

+  // If the types of all the generic bindings for the impl do not allow values
+  // with associated libraries, then no further specialization is possible in
+  // any other library, so the impl is effectively final.


I think this comment might need to be updated. It's not wrong but it's talking about the old context of this PR, but here we're checking for an impl being externally specializable, a somewhat inverse of effectively final

danakj · 2025-12-18T16:23:03Z

toolchain/check/testdata/impl/final_not_specializable.carbon

+
+// - adapter for such a type
+
+class Adapt { adapt i32; }


Can we test an Adapt of an array of i32?

danakj · 2025-12-18T16:26:28Z

toolchain/check/testdata/impl/final_not_specializable.carbon

+class A[T:! type](V:! T) {}
+
+// We can't tell whether a parameter of an incomplete type would make the impl
+// effectively final, but it doesn't matter because the impl is invalid anyway.


"would make the impl unable to be specialized externally"? Does this test still make sense?

danakj · 2025-12-18T16:28:12Z

toolchain/check/testdata/impl/lookup/int_literal_convert.carbon

+
+library "[[@TEST_NAME]]";
+
+fn ConvertZero(N:! Core.IntLiteral()) -> Core.Int(N) {


Could these tests get a comment explaining what they are testing (that the Core impl is final)?

github-actions · 2026-03-19T02:19:36Z

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please comment or remove the inactive label.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

zygoloid requested review from danakj and josh11b December 16, 2025 20:44

zygoloid requested a review from a team as a code owner December 16, 2025 20:44

github-actions bot added the toolchain label Dec 16, 2025

zygoloid added 2 commits December 16, 2025 21:00

Treat impls with no generic parameters of facet type as being effecti…

3e4d26e

…vely final. See carbon-language#6500 for details on why this is important.

Precompute effective finality.

bf71a11

Add testing.

zygoloid force-pushed the toolchain-effectively-final branch from 41317a3 to bf71a11 Compare December 16, 2025 21:11

danakj reviewed Dec 16, 2025

View reviewed changes

Avoid reusing a variable.

7ed91e3

Co-authored-by: Dana Jansens <danakj@orodu.net>

CarbonInfraBot reviewed Dec 17, 2025

View reviewed changes

Address some review comments

5312c22

josh11b removed their request for review December 17, 2025 23:06

Rework as a change to the rules on what can be declared final.

5527abf

zygoloid changed the title ~~Treat impls with sufficiently primitive generic parameters as being effectively final.~~ Allow impls with sufficiently primitive generic parameters to be declared final. Dec 17, 2025

Add tests for GetObjectRepr call.

39c6371

zygoloid requested a review from danakj December 17, 2025 23:26

danakj approved these changes Dec 18, 2025

View reviewed changes

github-actions bot added the inactive Issues and PRs which have been inactive for at least 90 days. label Mar 19, 2026

danakj added long term issue Issues expected to take over 90 days to resolve. Does not apply to PRs. and removed inactive Issues and PRs which have been inactive for at least 90 days. labels Mar 19, 2026

	SemIR::TypeId initial_type_id) -> bool {
	SemIR::TypeId initial_type_id)
	-> bool {


		library "[[@TEST_NAME]]";

		fn ConvertZero(N:! Core.IntLiteral()) -> Core.Int(N) {

Conversation

zygoloid commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danakj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zygoloid commented Dec 16, 2025 •

edited

Loading

danakj left a comment •

edited

Loading