-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[SR-13385] Improved diagnostics when using instance methods before initialization #55825
Comments
@swift-ci create |
Comment by Nikhil (JIRA) theindigamer (JIRA User) Is this for fixing and shall I give it a try. |
Interesting. Wow, these diagnostics are pretty old. We also have error: variable 'test' used before being initialized which we emit in certain cases. I'm not really sure why the "pass by reference" thing is being mentioned at all. Arguably that's not relevant. Anything that counts as a use is not permitted, which includes passing by reference. Also, we have a note note: variable defined here Maybe that note should be changed to As for assigning this to yourself, sure feel free to do that. For this one in particular, I don't see what the benefit is for a separate "pass by reference" diagnostic, so I'm in favor of removing it. Moreover, in certain cases, it leads to a duplicate diagnostic, which is not great (from the test suite): func testOptionalChainingWithGenerics<T: DIOptionalTestProtocol>(p: T) -> T? {
let x: T? // expected-note {{constant defined here}}
// expected-note@-1 {{constant defined here}}
// expected-note@-2 {{constant defined here}}
// note that here assignment to 'f' is a call to the setter.
x?.f = 0 // expected-error {{constant 'x' used before being initialized}}
// expected-error@-1 {{constant 'x' passed by reference before being initialized}}
return x // expected-error {{constant 'x' used before being initialized}}
} — Also, this diagnostic is not quite accurate. func f() -> Int {
var x: Int
if (true) {x = 0}
return x // error: variable 'x' used before being initialized
} Maybe as a follow up, we should revisit all the definite initialization diagnostics. Rust uses the term "possibly-uninitialized", maybe that's better? |
I think the “passed by reference” diagnostic is emitted for inout uses. It would be nice to point out where the inout access is happening (as notes) but I think it would be a little tricky. |
Could you explain how the additional "passed by reference" text (or pointing out where the inout access is happening) help someone understand what the issue is, or how to fix it? If I put the two diagnostics side-by-side (even including a hypothetical extension pointing out where the inout access is happening), I think "used before being initialized" conveys the same information ("hey! you need to initialize this before using it here") in fewer words. |
Hmm, my understanding was that the "passed by reference" bit was confusing here (not that it's not clear enough that the variable is uninitialized), at least in scenarios where it's happening behind-the-scenes like dictionary access. Normally, you'd see this diagnostic in scenarios like: func takesInout(_: inout Int) {}
var foo: Int
takesInout(&foo) // variable 'foo' passed by reference before being initialized There's another one for pointers: func takesPointer<T>(_: UnsafePointer<T>) {}
var foo: Int
takesPointer(&foo) // address of variable 'foo' taken before it is initialized Although for something like: var dict: [String: Int]
dict["a"] = 0 // variable 'dict' passed by reference before being initialized It's not immediately clear where the "pass by reference" bit is happening (I think it's happening via the synthesised But okay, perhaps all of this extra information is simply unnecessary (as you hinted earlier) and in that case I agree that changing the diagnostic wording would be better! |
@theblixguy theindigamer (JIRA User) I'm not really sure but to me this "pass by reference" bit might be happening because for mutating function/subscripts self is implicit passed as an inout parameter, so in the case of dict["a"] = 0, that would be something like subscript(&dict, "a", 0). So at SIL stage where I guess this diagnostic is being provided what is seeing its just an implicit inout access... That would be my guess, but I'm not really sure if that is the case 🙂 |
Seems like that is indeed the case and a simple change like // For a use of self argument implicit passed as inout parameter e.g.
// let's just diagnosed as "used before initialized" because "passed by
// reference" might be a confusing diagnostic in this context.
auto diagID = (Use.Kind == DIUseKind::InOutSelfArgument)
? diag::variable_used_before_initialized
: diag::variable_inout_before_initialized; here https://github.com/apple/swift/blob/303026b574227c2df5aecf2204f1d27f7b5ae5f9/lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp#L1128 improves the diagnostic. |
Additional Detail from JIRA
md5: de102270b8499a8c34615e7067e1329f
Issue Description:
One of the most common errors beginners of Swift like me make is to start using class instances just by declaring but not initializing.
For example:
In this case, the compiler does emit the below error:
variable 'test' passed by reference before being initialized
I think we can redact "passed by reference" cos Swift guide https://docs.swift.org/swift-book/LanguageGuide/Methods.html doesn't explicitly state that internally we pass object by reference and this SO thread https://stackoverflow.com/questions/24838015/variable-p-passed-by-reference-before-being-initialized gives us an interesting example where the user keeps asking why the phrase "passed by reference" is used.
The text was updated successfully, but these errors were encountered: