-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[DRAFT] Replacing 'synthesized initializers' with 'default initializers' on diagnostics #68856
base: main
Are you sure you want to change the base?
Conversation
Changing the term 'synthesized initializer' to 'default initializer' on the 3 diagnostics relating to missing initial values on stored properties of classes.
I was just testing the pull request on my own fork before requesting a real one soon, sorry for the review request, it was unintentional! |
I think it's relevant to discuss a more thorough rewriting of this note. Which is good because we are now using the same term as the documentation. But I would like to propose changing the message to: The reasons behind some of the changes are based on the two following excerpts from
Finally, two additional reasons are that to me the proposed message is more clear than the original and it will make it easier to learn about the concepts mentioned using the official documentation. |
@@ -1981,16 +1981,16 @@ NOTE(requires_stored_property_inits_here,none, | |||
ERROR(class_without_init,none, | |||
"%select{class|actor}0 %1 has no initializers", (bool, Type)) | |||
NOTE(note_no_in_class_init_1,none, | |||
"stored property %0 without initial value prevents synthesized " | |||
"stored property %0 without initial value prevents default " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Jordan notes here, the compiler term synthesized initializers encompasses both default and inherited initializers.
What if we rearchitect the diagnosis to avoid these terms altogether by pointing out that the class must implement an initializer because it has stored properties without default values? As a bonus, I believe this formulation would make for a far more helpful error message than the current class has no initializers
. The notes could then be adjusted to simply point at these stored properties instead of spelling out causal relationships.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally overlooked his comment! Thank you for highlighting it.
I'm not confident in my understanding of the relation with the inherited initializers, so I'll give an example and walk through it to check if I understood correctly.
class Superclass {
let boolean: Bool
// The compiler will synthesize a default initializer like init(boolean: Bool)
}
class Subclass: Superclass {
let text: String = "Example Text"
// The compiler will synthesize an inherited initializer like init(boolean: Bool), like the one seen above
}
If the text
variable didn't have an initial value, the inherited initializer wouldn't be able to be synthesized. But the changes I've made to the diagnostics would call the inherited initializer a default initializer, which is wrong.
Did I use the correct terms everywhere?
The solution then is to check if we are synthesizing an inherited initializer or a default initializer. And your suggestion is to change the current diagnostics to something along the lines of:
error: an initializer must be implemented because this class has stored properties without initial values
note: 'text' has no initial value
Did I understand the solution you proposed correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the
text
variable didn't have an initial value, the inherited initializer wouldn't be able to be synthesized. But the changes I've made to the diagnostics would call the inherited initializer a default initializer, which is wrong.
Yes, except a default initializer is an implicit initializer with no parameters. Memberwise initializers like init(boolean: Bool)
are synthesized only for structures. Inherited initializers can have parameters and are somewhat harder to define in terms of what actually happens. For a designated initializer, the inherited initializer is a distinct, implicit override thereof. Otherwise (convenience init
), it is the superclass initializer itself, and is called inherited because you can call it on the subclass. It’s easiest to simply think of them as initializers that are inherited per initializer inheritance rules.
Did I understand the solution you proposed correctly?
Sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: 'text' has no initial value
We spell these sorts of notes as <description> declared here
. It’s better not to phrase it as a complaint because we don’t know what course of action is best for the programmer, and we’d rather not mislead them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got it! Will try to implement and come back with it.
And regarding the note: makes sense, will use it.
As highlighted by this issue, the official documentation doesn't seem to use the word 'synthesized initializer' as some of the diagnostics do. It, instead, uses 'default initializers' to refer to this concept, as can be seen at the The Swift Programming Language (5.9) > Initialization > Default Initializers section.
This pull request simply replaces one term by the other on 3 diagnostics, which are related to eachother and seem to be the only ones using the 'synthesized' term.
Resolves #43376