Skip to content

[Diagnostics] Prioritize type mismatches over labeling failures for c… #36267

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 6, 2021
Merged

[Diagnostics] Prioritize type mismatches over labeling failures for c… #36267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd08350
[Diagnostics] Prioritize type mismatches over labeling failures for c…
xedin Mar 4, 2021
39b2ff9
[CSStep] Skip disabled overloads only if there are no fixes in diagno…
xedin Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9221,13 +9221,6 @@ bool ConstraintSystem::simplifyAppliedOverloadsImpl(
for (auto *choice : choices.slice(1))
choice->setDisabled();
}

// Don't attempt further optimization in "diagnostic mode" because
// in such mode we'd like to attempt all of the available overloads
// regardless of problems related to missing or extraneous labels
// and/or arguments.
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about this a little more and wondering - this is going to make the solver totally skip over overloads with labeling failures if it's able to fix up a different overload with correct labels, right? What's going to happen now if there's 1 label mismatch and N type mismatches in the expression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are overloads that have correct labels in expected positions than they'd be checked and ranked based on the number/kind of type failures they have, the rest is skipped, if there are no matching overloads - all of the overloads are going to be checked to determine what is going on (maybe there are out-of-order arguments or extraneous labels). The reasoning behind this is based on to the fact that name with incorrect labels shouldn't be found by the lookup anyway since labels are part of the name.

Copy link
Member

Choose a reason for hiding this comment

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

The reasoning behind this is based on to the fact that name with incorrect labels shouldn't be found by the lookup anyway since labels are part of the name.

That's true for a compound name, but not a function call. I believe lookup for a function call just uses the bare name (e.g. to get function calls working with $ labels for SE-0293, all I had to do was change argument-to-parameter label matching)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the case for all the names, since labels attached to the name in ABI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said lookup works on the bare name still and most likely will do so in the future.

Copy link
Member

@hborla hborla Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I understand that labels are part of the name for all names. I'm saying that lookup sometimes does not include the labels, and instead relies on argument-to-parameter matching.

Copy link
Member

Choose a reason for hiding this comment

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

What I am asking is - what diagnostic will be produced for code like this, which has only 1 label mismatch but several type mismatches?

func test(arg1: Int, arg2: Int) {}
func test(arg: String, arg2: String) {}

test(arg1: "", arg2: "")

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 understand what the concern is here, that's why it is working the way it is :) The idea behind these changes is that matching labels is strong enough indication that developer wanted a particular overload even though the types might not line up at the moment (e.g. they have been filled by completion with holes), so in your example it's going to be two type failures related to arguments instead of one label failure.

That said - there is still a possibility (and that's what your example indicates as well) that one of the labels has a typo, I think we could detect that during label matching and allow such overloads to be attempted in the diagnostic mode together with the ones where labels line up exactly.

if (solverState)
return false;
}

/// The common result type amongst all function overloads.
Expand Down
13 changes: 6 additions & 7 deletions lib/Sema/CSStep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,10 +598,6 @@ bool TypeChecker::isDeclRefinementOf(ValueDecl *declA, ValueDecl *declB) {
bool DisjunctionStep::shouldSkip(const DisjunctionChoice &choice) const {
auto &ctx = CS.getASTContext();

// Never skip disjunction choices in diagnostic mode.
if (CS.shouldAttemptFixes())
return false;

auto skip = [&](std::string reason) -> bool {
if (CS.isDebugMode()) {
auto &log = getDebugLogger();
Expand All @@ -613,11 +609,14 @@ bool DisjunctionStep::shouldSkip(const DisjunctionChoice &choice) const {
return true;
};

if (choice.isDisabled())

// Skip disabled overloads in the diagnostic mode if they do not have a
// fix attached to them e.g. overloads where labels didn't match up.
if (choice.isDisabled() && !(CS.shouldAttemptFixes() && choice.hasFix()))
return skip("disabled");

// Skip unavailable overloads.
if (choice.isUnavailable())
// Skip unavailable overloads (unless in dignostic mode).
if (choice.isUnavailable() && !CS.shouldAttemptFixes())
return skip("unavailable");

if (ctx.TypeCheckerOpts.DisableConstraintSolverPerformanceHacks)
Expand Down
2 changes: 1 addition & 1 deletion test/Constraints/generics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ let arr = [BottleLayout]()
let layout = BottleLayout(count:1)
let ix = arr.firstIndex(of:layout) // expected-error {{referencing instance method 'firstIndex(of:)' on 'Collection' requires that 'BottleLayout' conform to 'Equatable'}}

let _: () -> UInt8 = { .init("a" as Unicode.Scalar) } // expected-error {{missing argument label 'ascii:' in call}}
let _: () -> UInt8 = { .init("a" as Unicode.Scalar) } // expected-error {{initializer 'init(_:)' requires that 'Unicode.Scalar' conform to 'BinaryInteger'}}
Copy link
Collaborator

@xwu xwu Mar 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

😢 This change is a significant QoL regression for this diagnostic; the previous error was actually useful to the user, whereas this one is...not. Worse, a missing label is likely to be harder for a human being to discover on their own.

This is coming to my attention, incidentally, because #33889 will also cause this particular message to change; it's going to be brittle because where an initializer is implemented (default implementation versus on the concrete type) now seems to change the diagnostic message.

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 am working on fixing this!


// https://bugs.swift.org/browse/SR-9068
func compare<C: Collection, Key: Hashable, Value: Equatable>(c: C)
Expand Down
2 changes: 1 addition & 1 deletion test/Constraints/result_builder_diags.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ func erroneousSR11350(x: Int) {
if b {
acceptInt(0) { }
}
}).domap(0) // expected-error{{value of type '()?' has no member 'domap'}}
}).domap(0) // expected-error{{value of type 'Optional<()>' has no member 'domap'}}
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/stdlib/UnsafePointerDiagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func unsafeRawBufferPointerConversions(
_ = UnsafeRawBufferPointer(start: rp, count: 1)
_ = UnsafeMutableRawBufferPointer(mrbp)
_ = UnsafeRawBufferPointer(mrbp)
_ = UnsafeMutableRawBufferPointer(rbp) // expected-error {{missing argument label 'mutating:' in call}}
_ = UnsafeMutableRawBufferPointer(rbp) // expected-error {{cannot convert value of type 'UnsafeRawBufferPointer' to expected argument type 'UnsafeMutableRawBufferPointer'}}
_ = UnsafeRawBufferPointer(rbp)
_ = UnsafeMutableRawBufferPointer(mbpi)
_ = UnsafeRawBufferPointer(mbpi)
Expand Down
38 changes: 38 additions & 0 deletions validation-test/Sema/SwiftUI/rdar72771072.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// RUN: %target-typecheck-verify-swift -target x86_64-apple-macosx10.15 -swift-version 5
// REQUIRES: objc_interop
// REQUIRES: OS=macosx

import SwiftUI

enum E {
case a, b, c
}

struct S : View {
let values: [E] = [.a, .b, .c]

var body: some View {
ScrollView(.vertical) {
Group {
Group {
ForEach(values, id: \.self) { color in
Button(action: { labeled(true) }) { // expected-error {{missing argument label 'value:' in call}} {{38-38=value: }}
Text("").bold()
}.buttonStyle(BorderlessButtonStyle())
}

ForEach([1, 2, 3, 4, 5, 6, 7, 8, 9], id: \.self) { _ in
Button(action: { labeled(value: true) }) {
Text("").bold()
}.buttonStyle(BorderlessButtonStyle())
}
}
.frame(width: 100, height: 100)
.padding(.top, 1)
}
}
.frame(width: 100)
}

func labeled(value: Bool) {}
}
1 change: 0 additions & 1 deletion validation-test/Sema/type_checker_crashers_fixed/rdar45470505.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ extension BinaryInteger {
init(bytes: [UInt8]) { fatalError() }

init<S: Sequence>(bytes: S) where S.Iterator.Element == UInt8 {
// expected-note@-1 {{incorrect labels for candidate (have: '(_:)', expected: '(bytes:)')}}
self.init(bytes // expected-error {{no exact matches in call to initializer}}
// expected-note@-1 {{}}

Expand Down