Can property shadowing warnings be improved? (re: SR-6689)


(Marc Palmer) #1

Hi,

I have had discussions with several developers including some in Swift Users about an issue I raised around property shadowing: https://bugs.swift.org/browse/SR-6689 - I am informed it is not a bug, and that I should discuss possible changes to the language here so…

In a nutshell, I am trying to make some nice “convention-like” Swift APIs. This seems to be a nice thing to do, and protocol extensions arguably make this a Swifty way of doing things, as you can use them to provide defaults for “convention based” properties that consumers of your API can ignore unless they need them.

However, things get ugly really quickly due to the combination of type inference and the permitted shadowing of properties with differing types.

Here’s a quick example

protocol Action {
  static var parent: Action.Type? { get }
}
extension Action {
  static var parent: Action.Type? { return nil }
}
class MyAction: Action {
}

So far so good. My framework code can see MyAction.parent is nil. Now I hand this code over to public developers so they can create their own actions, and they very naturally do this:

class MyAction: Action {
  // Oops this is `Action` not `Action?`, incompatible with the protocol and shadows without a compiler error
  static var parent = OtherAction.self  
}

Then, if they are lucky (depending on what they assign there), they get compiler errors or weird behaviour, because Swift’s compiler permits the shadowing of the property that already has a definition in the protocol extension.

The compiler considers MyAction to conform to Action but if you access MyAction.parent explicitly in code you get a different value to whether you access it through a reference to a MyAction.

The developer can get very confused by this behaviour and the oblique compiler error messages (if they get them). There are two common scenarios that I’ve seen happen over and over again with myself working on this:

  1. you return non-optional forms of types when an optional is expected (very easy when using type inference on the property declaration)

and

  1. the problem of accidentally returning completely different types, which the compiler seems to permit.

I have been told this shadowing is desired / required behaviour. However I consider the current behaviour and compiler output around it to be very problematic for users of the compiler. Imagine getting problems like this in a Swift Playground on iPad ands trying to explain that to someone. Ouch.

I would therefore like to suggest that at the very least some compiler diagnostic messages are added to the effect of “Property ‘x’ in type ‘Y’ has a different type from the declaration in the protocol extension of ‘P’. Add the explicit type of the property to fix this”. - with a Fix-It option if possible.

I will say that I struggle to see, apart from perhaps a current implementation limitation, why there is value to this ability to shadow a property clearly required by a protocol to which the type intends to conform. The compiler assumes the type conforms to the protocol because of the protocol extension, and yet the type can have completely different property type.

If the protocol extension was missing it would fail to comply because it doesn’t conform so… ? I am bemused.


Concerned about this protocol extension/conformance bug
Classes should be disallowed from defining properties incompatible with their protocols...EVEN IF there is a default
(Erik Little) #2

This sounds like an extension to the work that’s already been done for near misses. Was there a reason this case was not covered in that work? At a glance I don’t see why we shouldn’t warn for these cases, as long as there’s a way to silence the warning by marking it as private.

Didn’t @Erica_Sadun have something similar to this that was much broader in scope? Yes, here.


(Xiaodi Wu) #3

Regarding your two scenarios:

Case 1: a non-optional implementation should fulfill a requirement of optional type; we lack true subtype relationships but this could be made to work; there’s a PR, outdated now, to make this possible for return types on method requirements, and there’s no reason it shouldn’t also apply to properties.

Case 2 is not an implementation limitation. Type properties must be allowed to shadow protocol requirements of a different type because adding new requirements to a protocol (as long as a default implementation is supplied too) must not break existing code that conforms to the protocol.


(Marc Palmer) #4

@xwu regarding case #2, bearing in mind I don’t have any real background info here: I don’t understand why that rule exists. It’s not about binary stability across dynamic frameworks is it, because at least on non-macOS platforms outside of the future ABI stable runtime there wouldn’t be scope for a “shared” library to change its API without the developer rebuilding their app.

If you use a new version of a protocol that introduces a new property that you’ve used in your own code, this should fail fast at compile time rather than lead to undefined and unpredictable behaviour the developer may not even know is happening.


(Marc Palmer) #5

Also @xwu if case #1 is to be tackled, what can I do to help bring that forward (short of making a PR myself as I simply do not have the C++/compiler chops for that). I really want to see these sharp edges fixed.


(Xiaodi Wu) #6

Re case 1, I thought the conclusion was that this was completing a settled design for the language, so short of a PR to implement there’s not much to be done…

Re case 2, it is in part about binary compatibility, which is a huge deal and the major focus of Swift 5. It’s also about source compatibility. We have to be able to write libraries that make meaningful incremental changes without breaking existing code, and adding a protocol requirement needs to be one of those things. The standard library itself relies on this feature; otherwise, any new protocol requirement we add would be source breaking, and the protocols of the standard library would be ossified in their current form. For example, we would not be able to add conditional conformances, because our hashValue would then break your hashValue. See the “library evolution” document in the Swift repository for more information on what library changes in Swift are intended to be compatible with existing code.


(Gwendal Roué) #7

I have problem 2, too. I’m currently addressing it with a documentation note:

☝️ Note: make sure the databaseSelection property is explicitely declared as [SQLSelectable]. If it is not, the Swift compiler may infer a type which may silently miss the protocol requirement, resulting in sticky SELECT * requests.


(Gwendal Roué) #8

Just like Marc, I wish that the compiler would emit a warning when type inference may conflict with protocol conformance. The warning would be removed by adding explicit type (either the protocol type, or the shadowing type). A fix-it with two options to choose from would be nice to have.


(Marc Palmer) #9

Bumping this - @gwendal.roue have you had any progress / found better solutions for conventions?

It makes me so sad. I would be happy for a fix-it.


(Gwendal Roué) #10

No Marc. We're just as the same point as we were when this thread was initiated, as far as I know!


(Adrian Zubarev) #11

I also had a question regarding case #1 a few days ago.

That mentioned, I also would like that the compiler would accept conformances with non-optional implementation satisfying optional requirement. This would, if possible to implement without any extra requirement, avoid issues with static dispatch which we currently can have.


(Marc Palmer) #12

Thanks for the confirmation Gwendal. I'd really like to be able to fix this in Swift myself but I have no idea where to start with either the "allow non-optional T literal property assignments to fulfil Optional requirements in protocols" or the "Include a warning or fix-it when assigning a non-optional T to a property from an adopted protocol of type Optional".

If someone from the Swift team could even give a hint as to what file in the Swift compiler these issues would be tackled, I would be grateful and this massively increases the chances of me actually finding and fixing the issue.