[Sema] Add specialization constraints for func and variable types, then diagnose w/fixes. #74909

gregomni · 2024-07-02T20:47:54Z

Similar logic to the diagnoses already happening in visitUnresolvedSpecializeExpr in CSGen, but checking again after there are no more type variables resolves #74857.

gregomni · 2024-07-02T20:48:05Z

@swift-ci Please smoke test.

AnthonyLatsis

Looks good, but I think we should be diagnosing more cases here, perhaps anything expect macro specializations and a DotSyntaxBaseIgnoredExpr subsexpression, which is this sort of case:

extension Bool {
  typealias A<T> = Bool
}

func foo(_ meta: Bool.Type) {
  let _ = meta.A<Bool>.self
}

lib/Sema/CSApply.cpp

test/Sema/generic-arg-list.swift

gregomni · 2024-07-17T14:05:37Z

@swift-ci Please smoke test.

gregomni · 2024-07-17T14:12:13Z

I think I've resolved the issues and solidified this. Now delays checking of all apparent function types and diagnoses them all afterwards in CSApply (or if we would otherwise get an unbound generic parameter error -- there).

Also, could you give me the gist of the request for this radar in test/decl/ext/specialize.swift: rdar://111059036
Is there a diagnostic wanted here specifically for trying to specialize Self? (Now easy to add, if so.)

xedin

A more reliable way to implement this would be to introduce the specialization constraint during CSGen regardless of what the base type is and use it to introduce fixes when types are sufficiently resolved.

Diagnosing something in CSApply is in general too late because it means that the solver produced an invalid solution which shouldn't happen for things that could be checked during solving.

gregomni · 2024-07-20T21:58:00Z

A more reliable way to implement this would be to introduce the specialization constraint during CSGen regardless of what the base type is and use it to introduce fixes when types are sufficiently resolved.

Done! I left the check in CSGen for known non-function types because diagnoses pointing at the type-parameter bracket is better for confusing situations like Parse/generic_disambiguation.swift:29:

var a, b, c, d : Int
(a < b, c > (d))

Where if you create the constraint you also get extraneous errors about b and c not being known types. But for everything else it now just creates the specialization constraint and diagnoses via the fix for it.

gregomni · 2024-07-20T21:58:08Z

@swift-ci Please smoke test.

xedin · 2024-07-21T01:18:53Z

Thanks! I will take a look on monday!

gregomni · 2024-07-21T04:00:28Z

Smoke test failed on Linux because a note was slightly different (different matching types on operator '<'). Replaced with substring of emitted note.

gregomni · 2024-07-21T04:00:44Z

@swift-ci Please smoke test.

gregomni · 2024-07-21T13:21:59Z

@swift-ci Please smoke test.

lib/Sema/CSGen.cpp

gregomni · 2024-07-21T19:00:30Z

Also added a commit that improves parsing to avoid many cases of unary postfix '>'.

@swift-ci Please smoke test.

lib/Sema/CSGen.cpp

test/Parse/enum_element_pattern_swift4.swift

lib/Sema/CSDiagnostics.h

xedin · 2024-07-22T21:41:42Z

lib/Parse/ParseType.cpp

-    return false;
+    // If this is the end of the expr (wouldn't match parseExprSequenceElement),
+    // prefer generic type list over an illegal unary postfix '>' operator.
+    return tok.isKeyword() && tok.getKind() != tok::kw_try;


@rintaro could you take a look at this please?

tok.isKeyword() is usually not suitable for detecting "the token is not related to the current production" because many decls starts with a non-keyword. e.g.

let a = Foo<Bar> open class Cls { ... }

P.isStartOfSwiftDecl() || P.isStartOfStmt() might be better.

gregomni · 2024-07-23T16:18:23Z

Did all the Sema review feedback. Will look at Parse when I get a chance -- or would it be easier if I did a separate PR for that part with a different reviewer?

@swift-ci Please smoke test.

xedin

I think we are super close here, left a few more comments inline, also we are focusing on methods/functions but what about subscripts? It would be good to add a few test-cases for them as well.

xedin · 2024-07-23T19:46:07Z

lib/Sema/CSGen.cpp

+        if (result->hasError()) {
+          auto &ctxt = CS.getASTContext();
+          auto *repr = new (ctxt) PlaceholderTypeRepr(specializationArg->getLoc());
+          result = PlaceholderType::get(ctxt, repr);


Instead of create a placeholder repr placeholder type should use original specializationArg

The PlaceholderType Originator argument requires specifically a PlaceholderTypeRepr, although the uses don't seem to require anything specific to that subclass, except that LocatorPathElt::PlaceholderType also requires it. So I could change both of those types to accept any TypeRepr. Is that what you mean?

It looks like replaceInferableTypesWithTypeVars does this same pattern of creating a PlaceholderTypeRepr, so I think this should be left as is.

That method is actually going the other way - it detects if there are placeholders in a Type and "opens" them. I think we should expand from PlaceholderTypeRepr to TypeRepr in PlaceholderType and locator element. But we can do that in a separate PR.

xedin · 2024-07-23T19:47:30Z

lib/Sema/CSGen.cpp

+      if (addSpecializationConstraint(overloadLocator, baseTy->getMetatypeInstanceType(),
+                                      expr->getUnresolvedParams())) {
+        // Diagnosis of failed type lookup - add a bit of context
+        auto &de = CS.getASTContext().Diags;


I still think we should delay simplification here to let "normal" solving fail and re-attempt during diagnostic mode.

Sorry, I think you'll have to be more explicit, I'm not sure I understand.

CS.addConstraint() happens inside of addSpecializationConstraint(). If this is changed to be CS.addUnsolvedConstraint(), then the solve for many of these cases will type check while leaving the specialization constraint in the Inactive list the whole time (because they really are extraneous), and then fail due to remaining inactive constraints (CSStep.cpp:482).

The if statement here was just for whether or not to add a note, the constraint is added no matter what. But I'll clean that part up. (So the note is emitted inside addSpecializationConstraint)

To be precise my suggestion was to remove the result type for addSpecializationConstraint, inside of that method: create a new ExplicitGenericArguments constraint, use addUnsolvedConstraint + activateConstraint (usually activateConstraint is not necessary but as far as I understand the main problem here that sometimes there are no type variables here for solver to bind and subsequently re-evaluate the constraint).

One trick to avoid explicit activation would be to allocate a new type variable and use an unsolved Equal constraint to connect it to the boundType, type variable itself should be used in place of boundType in an unsolved ExplicitGenericArguments constraint. This way the solver would pick the type variable and bind it during solving which activates all of the constraints that reference it.

Gotcha. Thanks for expanding.

lib/Sema/CSGen.cpp

lib/Sema/CSFix.cpp

gregomni · 2024-07-24T04:58:43Z

Okay! Feedback commit, round 2. Unfortunately, I have some questions about what you're asking for, so this isn't likely to be final.

gregomni · 2024-07-24T15:18:35Z

Ok, I think maybe done. Squashed changes so far.

@rintaro's suggestion of how to do the Parse change works perfectly.
The constraints are now added unsolved and then activated.

@swift-ci Please smoke test.

rintaro · 2024-07-24T18:34:02Z

@swift-ci Please smoke test

xedin

Thank you very much for all the work here!

xedin · 2024-07-24T21:06:44Z

lib/Sema/CSDiagnostics.cpp

@@ -9338,7 +9338,12 @@ bool InvalidMemberReferenceWithinInitAccessor::diagnoseAsError() {
 }

 bool ConcreteTypeSpecialization::diagnoseAsError() {
-  emitDiagnostic(diag::not_a_generic_type, ConcreteType);
+  emitDiagnostic(diag::not_a_generic_type, resolveType(ConcreteType));


Nit: I'd prefer the resolveType happened during construction of the diagnostic, it's usually safer that way.

Done. That was an accidental leftover from the changes along the way.

Always add constraints, find fixes during simplify. New separate fix for allow generic function specialization. Improve parse heuristic for isGenericTypeDisambiguatingToken.

gregomni · 2024-07-24T21:26:40Z

@swift-ci Please smoke test.

gregomni requested review from hborla, slavapestov and xedin as code owners July 2, 2024 20:47

AnthonyLatsis reviewed Jul 2, 2024

View reviewed changes

lib/Sema/CSApply.cpp Outdated Show resolved Hide resolved

test/Sema/generic-arg-list.swift Outdated Show resolved Hide resolved

test/Sema/generic-arg-list.swift Outdated Show resolved Hide resolved

test/Sema/generic-arg-list.swift Outdated Show resolved Hide resolved

gregomni force-pushed the generic-arg branch from 9d55400 to af9b68c Compare July 17, 2024 14:02

gregomni requested a review from AnthonyLatsis July 17, 2024 14:05

xedin requested changes Jul 18, 2024

View reviewed changes

gregomni force-pushed the generic-arg branch from af9b68c to ce7cb0a Compare July 20, 2024 21:45

gregomni requested review from ahoppen, bnbarham, CodaFi, DougGregor, hamishknight and rintaro as code owners July 20, 2024 21:45

gregomni changed the title ~~[Sema] Recheck UnresolvedSpecializeExpr after type variables are gone.~~ [Sema] Add specialization constraints for func and variable types, then diagnose w/fixes. Jul 20, 2024

gregomni requested a review from xedin July 20, 2024 22:47

gregomni force-pushed the generic-arg branch from ce7cb0a to 51939e1 Compare July 21, 2024 03:58

gregomni force-pushed the generic-arg branch from 51939e1 to 3810950 Compare July 21, 2024 13:21

hamishknight reviewed Jul 21, 2024

View reviewed changes

lib/Sema/CSGen.cpp Outdated Show resolved Hide resolved

gregomni force-pushed the generic-arg branch 2 times, most recently from b721de1 to ecbf057 Compare July 21, 2024 17:20

xedin reviewed Jul 22, 2024

View reviewed changes

gregomni requested a review from xedin July 23, 2024 19:33

xedin reviewed Jul 23, 2024

View reviewed changes

gregomni requested a review from xedin July 24, 2024 04:58

gregomni force-pushed the generic-arg branch from df2eb9d to 4d21511 Compare July 24, 2024 15:15

xedin approved these changes Jul 24, 2024

View reviewed changes

Improve diagnoses of generic specializations

6e917b5

Always add constraints, find fixes during simplify. New separate fix for allow generic function specialization. Improve parse heuristic for isGenericTypeDisambiguatingToken.

gregomni force-pushed the generic-arg branch from 4d21511 to 6e917b5 Compare July 24, 2024 21:25

gregomni merged commit 47acc09 into swiftlang:main Jul 25, 2024
3 checks passed

gregomni mentioned this pull request Jul 25, 2024

[Sema] Allow PlaceholderType Originator to be TypeRepr not just PlaceholderTypeRepr #75463

Closed

xedin mentioned this pull request Jul 25, 2024

Revert "[Sema] Add specialization constraints for func and variable types, then diagnose w/fixes." #75489

Merged

gregomni mentioned this pull request Jul 27, 2024

[Sema] Improve diagnoses of generic specializations #75526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sema] Add specialization constraints for func and variable types, then diagnose w/fixes. #74909

[Sema] Add specialization constraints for func and variable types, then diagnose w/fixes. #74909

gregomni commented Jul 2, 2024

gregomni commented Jul 2, 2024

AnthonyLatsis left a comment

gregomni commented Jul 17, 2024

gregomni commented Jul 17, 2024

xedin left a comment

gregomni commented Jul 20, 2024

gregomni commented Jul 20, 2024

xedin commented Jul 21, 2024

gregomni commented Jul 21, 2024

gregomni commented Jul 21, 2024

gregomni commented Jul 21, 2024

gregomni commented Jul 21, 2024

xedin Jul 22, 2024

rintaro Jul 23, 2024 •

edited

Loading

gregomni commented Jul 23, 2024

xedin left a comment

xedin Jul 23, 2024

gregomni Jul 24, 2024 •

edited

Loading

gregomni Jul 24, 2024

xedin Jul 24, 2024

xedin Jul 23, 2024

gregomni Jul 24, 2024 •

edited

Loading

xedin Jul 24, 2024

gregomni Jul 24, 2024

gregomni commented Jul 24, 2024

gregomni commented Jul 24, 2024

rintaro commented Jul 24, 2024

xedin left a comment

xedin Jul 24, 2024

gregomni Jul 24, 2024

gregomni commented Jul 24, 2024

[Sema] Add specialization constraints for func and variable types, then diagnose w/fixes. #74909

[Sema] Add specialization constraints for func and variable types, then diagnose w/fixes. #74909

Conversation

gregomni commented Jul 2, 2024

gregomni commented Jul 2, 2024

AnthonyLatsis left a comment

Choose a reason for hiding this comment

gregomni commented Jul 17, 2024

gregomni commented Jul 17, 2024

xedin left a comment

Choose a reason for hiding this comment

gregomni commented Jul 20, 2024

gregomni commented Jul 20, 2024

xedin commented Jul 21, 2024

gregomni commented Jul 21, 2024

gregomni commented Jul 21, 2024

gregomni commented Jul 21, 2024

gregomni commented Jul 21, 2024

Choose a reason for hiding this comment

rintaro Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

gregomni commented Jul 23, 2024

xedin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregomni Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregomni Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregomni commented Jul 24, 2024

gregomni commented Jul 24, 2024

rintaro commented Jul 24, 2024

xedin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregomni commented Jul 24, 2024

rintaro Jul 23, 2024 •

edited

Loading

gregomni Jul 24, 2024 •

edited

Loading

gregomni Jul 24, 2024 •

edited

Loading