swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.49k stars 10.35k forks source link

[SR-11277] Incorrect fix-it for property wrapper #53678

Open Agarunov opened 5 years ago

Agarunov commented 5 years ago
Previous ID SR-11277
Radar None
Original Reporter @Agarunov
Type Bug
Environment Swift 5.1 25.07.2019 Master 06.08.2019
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 0 | |Component/s | Compiler | |Labels | Bug, PropertyWrappers | |Assignee | None | |Priority | Medium | md5: 78089d8ad73de168d37631a50f8c728a

Issue Description:

This code provides incorrect fix-it:

class Value {
    @A(other: 0) var some: Int = 100 // error
    // Incorrect argument label in call (have 'wrappedValue:other:', expected 'initialValue:other:')
    // Replace '100' with 'initialValue'
}

@propertyWrapper
struct A<T> {
    var wrappedValue: T
    let other: T

    init(initialValue: T, other: T) {
        self.wrappedValue = initialValue
        self.other = other
    }
}

If I apply fix-it:

class Value {
    @A(other: 0) var some: Int = initialValue // error: Use of unresolved identifier 'initialValue'
}

If I change initialValue in init to wrappedValue everything compile correctly

theblixguy commented 5 years ago

The problem here is we have an implicit tuple expression (wrappedValue,other). wrappedValue comes from the assigned value which is why I think it seems to be pointing the fix-it to the value. There's a few ways to fix this:

1. Emit fix-its to insert `initialValue: 100` before `other`

  1. Emit fix-it to replace `initialValue` with `wrappedValue` in the wrapper (not sure how this will work if its in a different file, etc).
  2. Change the diagnostic text and remove the fix-it (similar to solution for SR-11060)

What do you think @xedin? Do you have any other ideas?

xedin commented 5 years ago

@theblixguy I think from usability standpoint #1 makes most sense, fix-it should form `@A(initialValue: 100, other: 0) var some: Int`

theblixguy commented 5 years ago

@xedin Sounds good! Although I don't have access to the variable inside diagnoseArgumentLabelError(). That is required to remove the '= 100' part. In LabelingFailure, we only have access to the CallExpr, but not the PatternBindingDecl.

xedin commented 5 years ago

Me and @DougGregor talked about this yesterday. It seems that the best option to solve this would be to actually start type-checking patterns via constraint solver.