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

Breakage related to default field values (RFC 3681) #993

Open
1 of 9 tasks
obi1kenobi opened this issue Nov 9, 2024 · 3 comments
Open
1 of 9 tasks

Breakage related to default field values (RFC 3681) #993

obi1kenobi opened this issue Nov 9, 2024 · 3 comments
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations

Comments

@obi1kenobi
Copy link
Owner

obi1kenobi commented Nov 9, 2024

RFC link; Tracking issue (not for discussion, just tracking): rust-lang/rust#132162

In short:

#[derive(Default)]
struct Pet {
    name: Option<String>, // impl Default for Pet will use Default::default() for name
    age: i128 = 42, // impl Default for Pet will use the literal 42 for age
}

// This works and produces:
// `Pet { name: Some(""), age: 42 }`
let _ = Pet { name: Some(String::new()), .. }

// Compilation error: `name` needs to be specified
let _ = Pet { .. }

Desirable SemVer major, deny by default lints:

  • default value removed from struct field, where the field continues to exist and the struct is still externally constructible with a literal
    • Witness: constructing the struct with { .. } syntax now requires supplying a value for that field.
    • It doesn't matter if the field is Default, since Default doesn't matter { .. }-style construction.
  • default value removed from enum struct variant field, where the field continues to exist and the variant is still constructible with a literal
    • Witness: constructing the variant with a literal now requires supplying a value for that field

Desirable SemVer major, warn by default lints:

  • default value changed on a struct field, from one explicitly-specified value to another
    • Reason to warn instead of error: it's possible the intended SemVer contract was "a default exists" rather than some specific value. It's situational, maintainers can promote this to error.
  • default value changed on an enum variant struct field, from one explicitly-specified value to another
    • Analogous to above case.
  • default value for a field (either struct or enum struct variant) changed from implicit Default::default() to a different, explicitly-specified value
    • Difficult! Need to determine if impl Default on the struct/enum is derived (hand-impls might not use the Default::default() for the field!), and also need to determine what Default::default() would return for the field's type. Default::default() isn't const in general, so we'd probably have to hardcode defaults for common built-in types.

Progress:

  • Waiting on RFC implementation in rustc
  • Waiting on rustdoc JSON to include field defaults
  • Need new schema in trustfall-rustdoc-adapter
  • Ready to write lints
@obi1kenobi obi1kenobi added A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations labels Nov 9, 2024
@obi1kenobi
Copy link
Owner Author

cc @estebank in case any SemVer edge cases I didn't notice jump out at you. No worries if you're too swamped with other work, just wanted to make you aware of this issue and give you the option to take a look! I like this feature and I'd like cargo-semver-checks support for it to be top-notch 😇

@estebank
Copy link

Default::default() is not currently const at all (but I quickly prototyped and it does work as expected).


Would you have thoughts about changing from one one way of producing the value to another while the value itself doesn't change?

Like

struct S {
    a: usize = 42,
}

to

struct S {
    a: usize = S::A,
}

impl S {
    const A: usize = 42;
}

@obi1kenobi
Copy link
Owner Author

Changing only how the value is produced doesn't seem breaking to me. I can't think of a "witness" program that demonstrates the breakage by working fine on one version and breaking on the next.

Is there a case where you think this might be breaking?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants