[Diagnostics] Emit a warning when an immutable decodable property has an initial value

[theblixguy]

When a decodable immutable property has an initial value, it will not be decoded and get skipped. This is a common source of confusion among users (example here and here), so emit a specialised warning for it.

struct Foo: Decodable {
  let someProperty: Int = 0 // warning because it won't be decoded
}

To silence the warning, the user can do one of the following things:

1. Set the initial value in the initializer (99% of the time this is what they really meant to do - set the value via the initializer by using it as a default argument)
2. Explicitly define a CodingKeys enum without the key in it to make it explicit that it's not meant to be decoded.
3. Make the property mutable (change let to var)

This won't trigger in some cases when the type is Codable because removing the key will break encoding which is most likely not what the user would expect.

Also, if the key has been explicitly specified and the type is just Decodable, then this warning will be triggered because explicitly declaring a key in the CodingKeys enum means it's meant to be decoded.

This strategy was mentioned here: https://forums.swift.org/t/revisit-synthesized-init-from-decoder-for-structs-with-default-property-values/12296/2

Resolves SR-11996
Resolves rdar://problem/58455438

[AnthonyLatsis]

Set the initial value in the initializer (99% of the time this is what they really meant to do - set the value via the initializer by using it as a default argument)

How would that work though? If the user really wants to leave the property as transient, the only other
way to show this without losing the synthesized conformance is to explicitly provide the coding keys. So when this happens, the question really is whether we want to encourage people to define coding keys by emitting the warnings.

[theblixguy]

I think it’s going to be rare in practice - most of the examples I’ve across on S/O, Twitter, etc is when someone just assigned it directly whereas they meant to pass it via the initializer.

Explicitly specifying the properties via CodingKeys is the alternative to this - I felt it was a bit too much for the common case but I’m totally happy to suggest that instead in the warning (or as a note in addition to making the property mutable).

[AnthonyLatsis]

There has to be both a "did you mean" fixit and a silencing one (which does not change behaviour or semantics). But I agree the common case should go first.
On the other hand, having to write additional code because of a warning when you see through that let and know what you're doing is a bit sad.

There is one case that definitely should be warned about though: when the intention to decode such a property is explicit:

struct Foo: Decodable {
  private enum CodingKeys: CodingKey {
    case foo1
  }

  let foo1 = 0
}

[theblixguy]

The main problem here is that explicitly implementing init(from:) will throw an error because the property cannot be overwritten, but the synthesised code does not throw an error because it just skips it. This difference in behaviour is what confuses people, it’s very magical/silent and there’s no help from the compiler to catch it. Now, according to my observation, most of the time the initial value was meant to be put in the initialiser, which is why I made the warning suggest that as the first choice and making the property mutable as the second.

But yeah, for those rare cases where you just want to ignore the property, explicitly specifying which properties to decode in a CodingKeys enum seems reasonable to me (my only concern was that suggesting it as the primary option might be overkill for the common case).

What I propose is that we suggest the initialiser approach as the first choice, coding keys as second choice and making the property mutable as the third choice.

Your example where you’ve explicitly specified the property in the CodingKeys enum but made it a let with initial value is a good example which should also be diagnosed as a warning.

[theblixguy]

Okay, I have made some tweaks and added more test cases. cc @xedin @hborla do you also mind taking a look? thank you!

[theblixguy]

Ping.

[xedin]

LGTM! Left a small comment about diagnose inline.

[theblixguy]

@swift-ci please smoke test

[theblixguy]

@swift-ci please smoke test

[Sameesunkaria]

I know that it has been a while since this has been merged but, have there been any considerations to have a way to disable this warning?

[BeauNouvelle]

Now that SwiftUI/Combine is in full swing, there are going to be more people using the let syntax for their Identifiable id conformance. Which I believe to now be the common case.

Having to create the CodingKeys enum to silence the warning is a bit silly.

If this change is here to stay, then perhaps a new property wrapper could help with silencing the warning.

Alternatively, fix Decodable so that it won't break if such a property with a default value set, isn't present within the incoming data.

[ptrkstr]

I agree with @BeauNouvelle.
This is becoming painful in SwiftUI and in some cases contributes to the opposite of reducing programmer error.
Take the following example:

struct Model: Codable, Identifiable {
    public let id = UUID() // warning presented
    public let name: String
}

Given that Xcode provides the Make the property mutable instead fix-it, we'll have developers ending up with this:

struct Model: Codable, Identifiable {
    public var id = UUID() // warning silenced
    public let name: String
}

It is poor practise to have a mutating identifier especially when SwiftUI relies heavily upon it.

Until this has been resolved (my hope would be that this merge is reverted), explicitly defining CodingKeys should not be recommended (in my opinion) as now a developer needs to remember to add/remove/modify/match the properties in their model (murphy's law states that a developer will eventually fail to maintain the coding keys).
Here is another workaround which boxes the model.

public struct IdentifierWrapper<T>: Identifiable {
    public let id = UUID()
    public let value: T
}

let wrapper = IdentifierWrapper(value: Model(name: "ptrkstr"))