Skip to content

Conversation

@jckarter
Copy link
Contributor

Builtin.FixedArray was introduced as the first generic builtin type, with special case handling in all the various recursive visitors. Introduce a base class, and move the handling to that base class, so it is easier to introduce other generic builtins in the future.

@jckarter
Copy link
Contributor Author

@swift-ci Please test

SubstitutionMap subs,
bool validate) {
if (!subs.getGenericSignature()->isEqual(getBuiltinGenericSignature(C, kind))) {
return ErrorType::get(BuiltinUnboundGenericType::get(kind, C));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be an ASSERT? What is the caller expected to do if they receive an ErrorType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried removing the ErrorType formation, and some diagnostic tests started failing, so it appears that some parts of Sema throw type bindings in here and rely on this logic for diagnostics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the ErrorType formation up to the affected callers then?

Copy link
Contributor

Choose a reason for hiding this comment

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

(It would also be cleaner if this method always returned the right type of Type, instead of just Type itself.)

Builtin.FixedArray was introduced as the first generic builtin type, with
special case handling in all the various recursive visitors. Introduce
a base class, and move the handling to that base class, so it is easier
to introduce other generic builtins in the future.
@jckarter jckarter force-pushed the builtin-generic-base-class branch from 8d58b68 to cf5f7c8 Compare October 28, 2025 19:48
@jckarter
Copy link
Contributor Author

@slavapestov I added caches and removed the assertion from CSSimplify; how's this look now?

I can look into chasing down the ErrorType clients in a follow-up.

Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

Alright, sounds good. Yeah we generally want to form ErrorTypes as close as possible "to the surface" because when they propagate around silently it can be difficult to debug the root cause of the problem.

@jckarter
Copy link
Contributor Author

@swift-ci Please test

@jckarter
Copy link
Contributor Author

@swift-ci Please test Windows

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants