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

[SR-13385] Improved diagnostics when using instance methods before initialization #55825

Open
swift-ci opened this issue Aug 13, 2020 · 8 comments
Open

Comments

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Aug 13, 2020

Previous ID SR-13385
Radar rdar://problem/67361258
Original Reporter srinikhil07 (JIRA User)
Type Improvement
Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Improvement, DiagnosticsQoI, StarterBug
Assignee None
Priority Medium

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:

var test : Dictionary<String, String>
 test.updateValue("1", forKey: "1")   
test["1"] = "1" // same error as below  

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.

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Aug 19, 2020

@swift-ci create

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Aug 22, 2020

Comment by Nikhil (JIRA)

theindigamer (JIRA User)

Is this for fixing and shall I give it a try.

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Aug 23, 2020

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 note: 'test' declared here but not initialized.

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?

@theblixguy
Copy link
Collaborator

@theblixguy theblixguy commented Aug 23, 2020

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.

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Aug 23, 2020

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.

@theblixguy
Copy link
Collaborator

@theblixguy theblixguy commented Aug 23, 2020

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 _modify accessor), so I thought a note explaining the inout access would provide some additional information (or it could be in the form of educational notes or pretty-printed rust style diagnostics).

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!

@LucianoPAlmeida
Copy link
Collaborator

@LucianoPAlmeida LucianoPAlmeida commented Aug 23, 2020

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

@LucianoPAlmeida
Copy link
Collaborator

@LucianoPAlmeida LucianoPAlmeida commented Aug 24, 2020

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

auto diagID = diag::variable_inout_before_initialized;
improves the diagnostic.
But as in the case you mention theindigamer (JIRA User) there indeed a duplication still, in this specific example one is maybe for the unwrap access and another for the setter(I don't have much knowledge on SIL diagnostics)... so maybe this should be re-visited

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants