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

[CS] Don’t fail constraint generation for ErrorExpr or if type fails to resolve #60062

Merged
merged 5 commits into from
Jul 20, 2022

Conversation

ahoppen
Copy link
Contributor

@ahoppen ahoppen commented Jul 14, 2022

Instead of failing constraint generation by returning nullptr for an ErrorExpr or returning a null type when a type fails to be resolved, return a fresh type variable. This allows the constraint solver to continue further and produce more meaningful diagnostics.

Most importantly, it allows us to produce a solution where previously constraint generation for a syntactic element had failed, which is required to type check multi-statement closures in result builders inside the constraint system.

Resolves: #60029
Resolves: #60485

@ahoppen ahoppen requested a review from xedin July 14, 2022 21:11
@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 14, 2022

@swift-ci Please smoke test

@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 14, 2022

@swift-ci Please SourceKit stress test

lib/Sema/CSGen.cpp Outdated Show resolved Hide resolved
@@ -11208,8 +11208,17 @@ ConstraintSystem::simplifyApplicableFnConstraint(
if (shouldAttemptFixes()) {
if (auto *typeVar = type2->getAs<TypeVariableType>()) {
auto *locator = typeVar->getImpl().getLocator();
if (typeVar->isPlaceholder() || hasFixFor(locator))
if (typeVar->isPlaceholder() || hasFixFor(locator)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please do a couple of things here:

  • Remove typeVar->isPlaceholder() check since it's always false because type variable cannot be a placeholder anymore
  • Move whole if (shouldAttemptFixes()) to just before TypeMatchOptions subflags = getDefaultDecompositionOptions(flags);
  • use desugar2 instead of simplifyType(type2);

Copy link
Contributor Author

@ahoppen ahoppen Jul 15, 2022

Choose a reason for hiding this comment

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

use desugar2 instead of simplifyType(type2);

We need to simplify the type because getFixedTypeRecursive doesn’t simplify AnyMetatypeTypes that have a type variable as instance type, so in desugar2 the type variable won’t have been replaced by a placeholder yet. The alternative would be to add handling for AnyMetatypeType in getFixedTypeRecursive. Do you have a preference what would be the better solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I keep forgetting about all the nuances of getFixedTypeRecursive vs. simplifyType. Maybe we should just call getFixedTypeRecursive on an instance type if type2 was a metatype for this new check.

test/stmt/if_while_var.swift Outdated Show resolved Hide resolved
test/Sema/placeholder_type.swift Show resolved Hide resolved
lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved
lib/Sema/CSDiagnostics.cpp Outdated Show resolved Hide resolved
@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 15, 2022

@swift-ci Please smoke test macOS

@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 15, 2022

@swift-ci Please SourceKit stress test

@ahoppen ahoppen force-pushed the pr/placeholder-for-errors branch 3 times, most recently from d3a70ab to 95abc5b Compare July 18, 2022 09:49
@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 18, 2022

@swift-ci Please smoke test macOS

if case let .Naught(value1, value2, value3) = n {} // expected-error{{pattern with associated values does not match enum case 'Naught'}}
// expected-note@-1 {{remove associated values to make the pattern match}} {{20-44=}}
// expected-error@-2 {{could not infer type of variable 'value1'}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is actually a regression of sorts - the main problem here is described by the first error and problems with value1..3 are just a consequence...


for (x, y, _) in arr2 {}
// expected-error@-1 {{pattern cannot match values of type '(a: Int, b: String)'}}
// expected-error@-2 {{could not infer type of variable 'x'}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the same, maybe we could somehow coalesce fixes

@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 19, 2022

@swift-ci Please smoke test

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! I have left a couple more minor comments inline.

lib/Sema/CSBindings.cpp Outdated Show resolved Hide resolved
lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved
@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 19, 2022

@swift-ci Please smoke test

…to resolve

Instead of failing constraint generation by returning `nullptr` for an `ErrorExpr` or returning a null type when a type fails to be resolved, return a fresh type variable. This allows the constraint solver to continue further and produce more meaningful diagnostics.

Most importantly, it allows us to produce a solution where previously constraint generation for a syntactic element had failed, which is required to type check multi-statement closures in result builders inside the constraint system.
…nside a named pattern

We need this to resolve a test failure in optional.swift.
The issue has been fixed by allowing holes for variables in paterns.
@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 20, 2022

@swift-ci Please smoke test

@ahoppen ahoppen merged commit 57d504c into swiftlang:main Jul 20, 2022
@ahoppen ahoppen deleted the pr/placeholder-for-errors branch July 20, 2022 12:15
@LucianoPAlmeida
Copy link
Contributor

Resolves: #60485

ahoppen added a commit to ahoppen/swift that referenced this pull request Jan 4, 2023
ahoppen added a commit to ahoppen/swift that referenced this pull request Jan 4, 2023
ahoppen added a commit to ahoppen/swift that referenced this pull request Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants