[Sema][NFC] Correcting test case for SR-11535 and adding an explanation comment

[LucianoPAlmeida]

Correcting the test case already added and adding an explanation comment.

cc @Jumhyn

[LucianoPAlmeida]

@swift-ci Please smoke test

[Jumhyn]

Do you think we should try to offer a warning here (“assuming you mean GenericEnumWithStaticNone.none, did you mean Optional.none instead?”) like we do for the inverse case?

[LucianoPAlmeida]

Do you think we should try to offer a warning here (“assuming you mean GenericEnumWithStaticNone.none, did you mean Optional.none instead?”) like we do for the inverse case?

Not really sure, I think the warning only makes sense if we have ambiguity and the one was favored(because the other one is a valid option too). Also, in this case, fix-it to Optional.none will not going to work because it leads to 'T' cannot be inferred although is not obvious since we didn't notice, if we added a fix-it it probably would have to be something like Optional<<#placeholder#>>.none but I don't if this is worth it in this case.

[Jumhyn]

Yeah, I could go either way on this. While there might not be strictly be ambiguity, I think for any readers of the code it’s likely that .none looks ambiguous, in that it’s hard to tell whether it refers to Optional.none or Wrapped.none without some serious thought.

Also, we could offer a fix-it which uses the same type parameter we’ve actually resolved .none to, since we’ll only reach that point if we actually have a valid solution.

[LucianoPAlmeida]

Yeah, I could go either way on this. While there might not be strictly be ambiguity, I think for any readers of the code it’s likely that .none looks ambiguous, in that it’s hard to tell whether it refers to Optional.none or Wrapped.none without some serious thought.

That makes sense, but I wonder if users that intentionally try to do that relying on this since is correct behavior, will not be able to use type inference without a warning. So we will be kinda enforcing to always use explicit type annotation when accessing .none members with optional contextual type... but, that may be just a point of view, do you think this makes sense?
To me, this seems the kind of warning that if added users should be able to enable/disable using some frontend flag.
In a way like they can tell the compiler if they want this kind of enforcement :)

[Jumhyn]

So we will be kinda enforcing to always use explicit type annotation when accessing .none members with optional contextual type... but, that may be just a point of view, do you think this makes sense?

If we do offer a warning here, I think it should only be present when the contextual type of the .none member is optional. That is, with the following setup:

struct Foo<T> {
  static var none = Foo<Int>()
}

let _: Foo? = .none
let _: Foo = .none

The first line should generate a warning, but the second line should be fine. My personal opinion is that diagnostics, in addition to guiding users to write correct code, should also guide them to write readable code. If we can identify cases where there's a potential ambiguity to the reader, we should try to mitigate that.

We don't allow the user to silence the warning generated when .none is interpreted as Optional.none over Wrapped.none, so I don't see a super strong case for why this warning should be able to be silenced.

This is all very subjective, though! I don't feel very strongly either way between warning/no warning.

[LucianoPAlmeida]

The first line should generate a warning, but the second line should be fine. My personal opinion is that diagnostics, in addition to guiding users to write correct code, should also guide them to write readable code. If we can identify cases where there's a potential ambiguity to the reader, we should try to mitigate that.

That totally makes sense :)
But as you said this can be subjective, I can see users preferring not to have a warning in those situations...

I took a shot at implementing the warning, so let's hear from others and see if this may be a good option, otherwise, we can just drop the last commit and move-on just updating the test case.

cc @xedin @hborla

[xedin]

Instead of discovering this post-factum how about we just convert this into a warning fix and detect the issue in simplifyMemberConstraint?

[LucianoPAlmeida]

Instead of discovering this post-factum how about we just convert this into a warning fix and detect the issue in simplifyMemberConstraint?

Right! I'm on it :)

[LucianoPAlmeida]

The latest changes are dependent on member lookup changes from #34715

[Jumhyn]

@LucianoPAlmeida Sorry for the delay on my PR... I've had less free time than expected over the past couple weeks. If you have any desire to take a crack at writing the requested unit test yourself I would happily accept a pull request into my branch, otherwise I will try to get to it by next week.

[LucianoPAlmeida]

@LucianoPAlmeida Sorry for the delay on my PR... I've had less free time than expected over the past couple weeks. If you have any desire to take a crack at writing the requested unit test yourself I would happily accept a pull request into my branch, otherwise I will try to get to it by next week.

@Jumhyn Don't worry, we can wait for it no problem :)

[Jumhyn]

@LucianoPAlmeida My PR has been merged so you should be unblocked on this one now :)

[LucianoPAlmeida]

@LucianoPAlmeida My PR has been merged so you should be unblocked on this one now :)

@Jumhyn Awesome! Thanks

[xedin]

LGTM! I have left a couple of minor suggestions inline.

[LucianoPAlmeida]

@swift-ci Please smoke test

[LucianoPAlmeida]

@xedin Any other point or is good to go?

[xedin]

[LucianoPAlmeida]

Thank you :)