[Sema] Suppress deprecation warnings in synthesized code

[AnthonyLatsis]

Resolves SR-8634.

[AnthonyLatsis]

cc @xedin

[jrose-apple]

@CodaFi has brought this up before and I'm not sure it's correct. We want to know in general if the compiler is synthesizing incorrect code.

[AnthonyLatsis]

@jrose-apple Right, the solution is not to synthesize at all if something is unavailable, and synthesize with possible corrections if something is deprecated. But if there aren't any deprecation corrections passed with @available we can rely on, we have to either refrain from synthesizing or suppress the upcoming warning in TypeCheckAvailability.

For example, should we synthesize rawValue here? Because it's a warning we're talking about, I think we should. Besides, we'd be leaving .b and .c out as well if we don't.

enum Foo: Int {
  @available(*, deprecated)
  case a
  case b
  case c
}

[AnthonyLatsis]

I just found out this doesn't compile:

enum Foo: Int {
  @available(*, unavailable)
  case a
  case b
  case c
}

I'm not sure what Foo.b.rawValue should be equal to, 0 or 1?

[jrose-apple]

Oh, I agree we need to synthesize rawValue to handle a (and init(rawValue:)), and I agree that it shouldn't warn, and I know we don't have an equivalent of GCC's __extension__ to silence the warning locally…but I don't know if we want to silence this for all synthesized declarations ahead of time. It seems too easy for us to miss something, like if we inherited initializers by synthesizing super.init calls and forgot to mark the inherited initializer as deprecated.

One possibility would be to come up with a compiler-only attribute to silence deprecation warnings, just like. We can put that on synthesized declarations that we know can reference deprecated members without themselves being deprecated.

Maybe I'm just being oversensitive, though.

---

I don't know what to do about the unavailable case. I'd suggest that it still claims a number, since that's most compatible with "I had an existing enum and I added unavailable to one case". That might be worth addressing in a separate PR, though, and probably then a second SR too.

[AnthonyLatsis]

but I don't know if we want to silence this for all synthesized declarations ahead of time. It seems too easy for us to miss something, like if we inherited initializers by synthesizing super.init calls and forgot to mark the inherited initializer as deprecated.

I don't really understand how an emitted warning will help here, or why synthesized declarations that are themselves deprecated for some reason would cause a problem if the warning isn't emitted. Could you elaborate a bit? First of all, we definitely don't want to emit it in synthesized code, right?

[jrose-apple]

Here's the example:

class Base {
  init(good: Int) { }
  @available(*, deprecated) init(bad: Int) { }
}

class Sub: Base {
  // initializers inherited
}

Sub(bad: 0) // should warn

If the compiler implemented this by synthesizing this declaration in Sub:

  init(bad: Int) {
    super.init(bad: bad)
  }

then we'd get a warning today telling us that that super.init call was wrong. The correct fix in this case would be to make sure the synthesized declaration was also marked deprecated:

  @available(*, deprecated)
  init(bad: Int) {
    super.init(bad: bad)
  }

Silencing the warning in all synthesized code would make it less likely that we'd notice this issue; we'd have to wait for a bug report from outside with the original test case.

I'm not sure if this is sufficient justification to look for a different answer, but it is why I'm worried.

[AnthonyLatsis]

I get your point now, thanks. Inheritance is probably the only case when we preserve availability in synthesized code. I feel like ensuring we have a test for that is a better option than adding a new internal attribute. In both cases it all boils down to having an appropriate test if we add anything new regarding code synthesis, so there's only a subtle difference in safety.

[jrose-apple]

We've had problems in the past with members synthesized in the Clang importer too. The point is that if we forget to test something, are we more likely to notice a warning that wasn't expected or a missing warning that should have been present?

…but then again, the people noticing are probably our users, and they'd have to wait for the next release to make the warning go away. So okay.

[jrose-apple]

@swift-ci Please test

[jrose-apple]

@swift-ci Please test source compatibility

[AnthonyLatsis]

I agree that the presence of an unexpected warning is more likely to be noticed, and I perfectly understand why you're worried. It's hard for me to tell what would be better at this point, although it does fix the bug without complicating the compiler. If we decide on landing this, let's at least add a test for Sub(bad: 0) // should warn, I haven't found one. I'm not familiar with what's going on in the Clang Importer, but if there's anything that concerns you, I can look for specific tests.

[jrose-apple]

It's good to add the test, but note that in this case it's not actually related, since we don't synthesize bodies for inherited initializers in Sema. (We take care of it in SILGen.)

@swift-ci Please smoke test

[zx2c4]

@jrose-apple Any plans to backport this to Swift 4.2.x?

[jrose-apple]

We generally don't backport fixes to earlier releases if they're not being released for all platforms (especially when there are already beta releases for 5.0), but if this is important to you for Linux you could cherry-pick it and open a PR per the 4.2.x process forum post.

[zx2c4]

if this is important to you for Linux

Actually, no -- I'm writing an iOS/macOS app that uses the LAError enum and was hoping those warnings would go away in Xcode.

[jrose-apple]

Ah, then you'll have to wait for Xcode 10.2 to GM.