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

[Diagnostics] Introduce argument-to-parameter mismatch #27063

Merged
merged 38 commits into from Sep 16, 2019

Conversation

xedin
Copy link
Member

@xedin xedin commented Sep 6, 2019

This PR aims to port argument-to-parameter conversion failures in their most general form cannot convert X to expected argument Y and some port tailored diagnostics related to them, lift some of the restrictions on use of requirement failures and remove now obsolete logic from CSDiag.

Resolves: rdar://problem/54613698
Resolves: rdar://problem/55254652
Resolves: rdar://problem/51262694
Resolves: rdar://problem/52334290
Resolves: rdar://problem/17647738
Resolves: rdar://problem/18624434
Resolves: rdar://problem/19979645
Resolves: rdar://problem/21032147
Resolves: rdar://problem/22042549
Resolves: rdar://problem/25639952
Resolves: rdar://problem/28047208
Resolves: rdar://problem/28555798
Resolves: rdar://problem/28675189
Resolves: rdar://problem/29214149
Resolves: rdar://problem/31785308
Resolves: rdar://problem/32390841
Resolves: rdar://problem/33914446
Resolves: rdar://problem/41303412
Resolves: rdar://problem/35420209
Resolves: rdar://problem/21948825
Resolves: SR-11388
Resolves: SR-4795

@xedin
Copy link
Member Author

xedin commented Sep 6, 2019

This is still a draft, 10+ tests are failing, mainly due to changes related to binary operator <op> cannot be applied to operands of type 'X' and 'Y' diagnostic, which I need to port to diagnoseAmbiguityWithFixes.

Copy link
Member

@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.

The new diagnostics are looking much better overall. Excellent work!

lib/Sema/CSDiag.cpp Show resolved Hide resolved
@xedin xedin force-pushed the diag-arg-conversion-failure branch from 8fd606c to d5996ea Compare September 7, 2019 05:46
@xedin
Copy link
Member Author

xedin commented Sep 13, 2019

@swift-ci please test

@xedin
Copy link
Member Author

xedin commented Sep 13, 2019

@swift-ci please test source compatibility

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - 0f95fe76aef2aa097acaf73ae2146ca7388a65db

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - 0f95fe76aef2aa097acaf73ae2146ca7388a65db

@xedin
Copy link
Member Author

xedin commented Sep 13, 2019

Please test with following pull request:
apple/swift-package-manager#2338

@swift-ci please test

@xedin
Copy link
Member Author

xedin commented Sep 14, 2019

@shahmishal Doesn't look source compatibility suite failure in CoreStore is related to my changes:

/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/project_cache/CoreStore/build/Build/Products/Debug-iphoneos/CoreStore.framework/Headers/CoreStore-Swift.h:1:2: error: unterminated conditional directive
#if 0
 ^
/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/project_cache/CoreStore/build/Build/Products/Debug-iphoneos/CoreStore.framework/Headers/CoreStore-Swift.h:4151:37: error: missing '@end'
/// \param object the entity type fo
                                    ^
/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/project_cache/CoreStore/build/Build/Products/Debug-iphoneos/CoreStore.framework/Headers/CoreStore-Swift.h:4138:1: note: protocol started here
@protocol CSListObjectObserver <CSListObserver>
^

/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/project_cache/CoreStore/Sources/CoreStoreBridge.m:182:13: error: receiver type 'CSSelect' for instance message is a forward declaration
    return [[CSSelect alloc] initWithDataTerm:selectTerm];
            ^~~~~~~~~~~~~~~~
In file included from /Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/project_cache/CoreStore/Sources/CoreStoreBridge.m:27:
/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/project_cache/CoreStore/Sources/CoreStoreBridge.h:362:8: note: forward declaration of class here
@class CSSelect;
       ^

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This is fantastic!

lib/Sema/ConstraintSystem.h Outdated Show resolved Hide resolved
test/Constraints/bridging.swift Outdated Show resolved Hide resolved
func f19997471(_ x: String) {}
func f19997471(_ x: Int) {}
func f19997471(_ x: String) {} // expected-note {{candidate expects value of type 'String' at position #0}}
func f19997471(_ x: Int) {} // expected-note {{candidate expects value of type 'Int' at position #0}}
Copy link
Member

Choose a reason for hiding this comment

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

NICE!


func someGeneric19997471<T>(_ x: T) {
takeVoidVoidFn {
f19997471(x) // expected-error {{cannot invoke 'f19997471' with an argument list of type '(T)'}}
// expected-note @-1 {{overloads for 'f19997471' exist with these partially matching parameter lists: (Int), (String)}}
f19997471(x) // expected-error {{no exact matches in call to global function 'f19997471'}}
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't to be this pull request, but it would be really nice if we reminded the user of the types of the arguments here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it would be better to attach arguments to this diagnostic or to each note?

test/Constraints/diagnostics.swift Outdated Show resolved Hide resolved
// Once argument-to-parameter mismatch diagnostics are moved to the new diagnostic framework, we'll be able to restore
// original contextual conversion failure diagnostic here. Note that this only happens in Swift 4 mode.
elems.deinitialize(count: self.value) // expected-error {{ambiguous reference to member 'deinitialize(count:)'}}
elems.deinitialize(count: self.value) // expected-error {{cannot convert value of type 'UInt' to expected argument type 'Int'}}
Copy link
Member

Choose a reason for hiding this comment

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

Great!

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - bcd4180f302967155f78dc27708d1db8352561a3

@xedin
Copy link
Member Author

xedin commented Sep 14, 2019

Please test with following pull request:
apple/swift-package-manager#2338

@swift-ci please test

@xedin
Copy link
Member Author

xedin commented Sep 14, 2019

Ugh, have to resolve a conflict in test/ParseableInterface/Inputs/opaque-result-types-client.swift now...

No specialized diagnostics yet, just a basic infrastructure to
produce generic a `cannot convert X to expected argument type Y`
message.
… down

Since this is the most generic failure, we need to make sure that
it doesn't interfere with other more specific fixes.
…ions

Most of the fix-its associated with contextual failures are also
appropriate for argument-to-parameter conversion mismatches.
…sitions

There is logic in `matchTypes` which would unwrap l-value if other
type is not an `inout`, which is not great for cases where parameter
type is a pointer and argument is an l-value which requires explicit
`&` to be converted.
… be resolved

Currently in cases where dependent member types cannot be resolved
properly due to invalid base type they do not fail comparison, but
instead result in a new "inactive" constraint which is incorrect.
… of invalid expressions

If expression is incorrect it most likely wouldn't be able to satisfy
`Equatable` or other requirements of `~=` operator overloads, but
at the same time the main problem is related to `case` expression
itself so let's not diagnose missing conformances.
In cases when all of the fixed solutions have only one problem in
common - different overloads of a certain operator, let's
produce a tailored diagnostic and suggest matching partial
overloads along side diagnostic notes which point to each choice.
…in diagnostic mode

Some spots of constraint simplification logic are too eager to
fail right away without giving repair logic to run in diagnostic
mode and attempt to fix the problem. Failing early in diagnostic
mode means solver wouldn't be able to reach some possible
solutions which leads to subpar diagnostics.
…missing conformance

If missing conformance is between two stdlib defined types which
are used in operator invocation, let's produce a generic diagnostic
about operator reference and a note about missing conformance.
…meter is a collection

This allows better diagnostics in cases where collection element
is a generic parameter which is supposed to be inferred from argument e.g.

```swift
func foo<T>(_: [T]) {}

foo(1) // Missing brackets, so error should be about [Int] vs. Int
```
…extual

Impact is higher if the base is expected to be inferred from context,
because a failure to find a member ultimately means that base type is
not a match in this case.
… diagnostic location

If one of the arguments to binary operator is invalid, let's
verify that diagnostic points directly to it.
@xedin
Copy link
Member Author

xedin commented Sep 14, 2019

Please test with following pull request:
apple/swift-package-manager#2338

@swift-ci please test

@apple apple deleted a comment from swift-ci Sep 14, 2019
@apple apple deleted a comment from swift-ci Sep 14, 2019
@apple apple deleted a comment from swift-ci Sep 14, 2019
@apple apple deleted a comment from swift-ci Sep 14, 2019
@DougGregor
Copy link
Member

W00t! Merge it!

@xedin
Copy link
Member Author

xedin commented Sep 16, 2019

Alright, let's run the tests one more time and :shipit:

@xedin
Copy link
Member Author

xedin commented Sep 16, 2019

Please test with following pull request:
apple/swift-package-manager#2338

@swift-ci please test

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.

None yet

5 participants