apple / swift-protobuf

Plugin and runtime library for using protobuf with Swift
Apache License 2.0
4.59k stars 459 forks source link

Support deprecated annotation on messages, fields, enums, enum values. #151

Open thomasvl opened 7 years ago

thomasvl commented 7 years ago

descriptor.proto defines a deprecated bool on message, enum, enum value, field; the generated things should get the Swift annotations also.

note: Extensions should honor it since they are a field.

thomasvl commented 7 years ago

Ok, this one is turning into a complex problem. ☹️

The annotations can go on the file, messages, enum, enum values, and fields. Spitting out the @availablity() line is easy enough. The hard part is then the rest of the code generated and what happens when it compiles.

All the other code we generate (for isEqual, hash, etc. can run into trying to access things that are now tagged as deprecated and the compiler spits out warnings. For C++/ObjC/etc. we work around this by using #pragma to push/pop the warnings on/off in the code we generate so the code will compiled silently, but the usage by developers will get the warnings. But I don't see a way to do something like this.

For the fields, I can cheat and always ensure a deprecated field gets a _name private field that all the code uses and only the public _var name` gets tagged as deprecated.

But when it comes to tagging enums and structs for messages, I don't see any way to cheat, they can be deprecated but still used in a deprecated field of another message also; and so that seems like it will always generate warnings when the code is compiled.

It doesn't seem right to say the generated code won't compile warning free, so I'm not sure how we support deprecated…

thomasvl commented 7 years ago

Un-assigning this at the moment. I think we need to support this, but if also seems like Swift doesn't have the support to do this correctly for Message/Enum/Enum Values, and if we generate code that doesn't compile cleanly, it will probably result in bugs against this project.

allevato commented 7 years ago

I've filed SR-3357 to start a discussion about easing up on deprecation warnings originating from references within the same compilation unit or module, to see if the language itself can help here.

thomasvl commented 7 years ago

Moving this to an enhancement, given the current support in Swift, it does't seem like we can do this and have the generate code compile (cleanly), so until that support is improved, we likely have to put this off.

SebastianThiebaud commented 6 years ago

Any update on this or the limitations remain?

thomasvl commented 6 years ago

SR-3357 doesn't have any updates, so without it, we wouldn't be able to generate something that would actually compile cleanly in the first place.

tbkka commented 5 years ago

Any updates? Since #861 is opening the door for a possible major version bump, we should consider whether it's time to do this.

thomasvl commented 5 years ago

I haven't seen any one pick up the issue for Swift. Maybe with a stable ABI more folks will start to see how the current support for deprecated annotations in Swift doesn't work for library authors.

DanF-Go commented 1 year ago

If we can't do deprecation, at least generate a documentation comment.

thomasvl commented 1 year ago

If we can't do deprecation, at least generate a documentation comment.

Sure. Most protos I've seen add something in the comments, but it can't hurt to also stick something else in after any comments pulled over from the .proto file.

DanF-Go commented 1 year ago

Ok, this one is turning into a complex problem. ☹️

The annotations can go on the file, messages, enum, enum values, and fields. Spitting out the @availablity() line is easy enough. The hard part is then the rest of the code generated and what happens when it compiles.

All the other code we generate (for isEqual, hash, etc. can run into trying to access things that are now tagged as deprecated and the compiler spits out warnings. For C++/ObjC/etc. we work around this by using #pragma to push/pop the warnings on/off in the code we generate so the code will compiled silently, but the usage by developers will get the warnings. But I don't see a way to do something like this.

For the fields, I can cheat and always ensure a deprecated field gets a _name private field that all the code uses and only the public _var name` gets tagged as deprecated.

But when it comes to tagging enums and structs for messages, I don't see any way to cheat, they can be deprecated but still used in a deprecated field of another message also; and so that seems like it will always generate warnings when the code is compiled.

It doesn't seem right to say the generated code won't compile warning free, so I'm not sure how we support deprecated…

For a softer deprecation, we could add annotation like

@available(swift, deprecated: 100.0)

which doesn't spit out compiler warning but still let developer know it's deprecated.

thomasvl commented 1 year ago

Does Xcode give any indication based on that? Or did you mean just because it it is something in the generated source?

The comment idea is a good one since it should show up in IDEs with any copied over documentation comments.

thomasvl commented 1 year ago

Does Xcode give any indication based on that? Or did you mean just because it it is something in the generated source?

The comment idea is a good one since it should show up in IDEs with any copied over documentation comments.

Doing a quick test, it seems like nothing shows up within Xcode including quick help, so developers would have to look at the generated sources to see it. For that reason, seems like something added to the doc string might be better.

DanF-Go commented 1 year ago

Does Xcode give any indication based on that? Or did you mean just because it it is something in the generated source? The comment idea is a good one since it should show up in IDEs with any copied over documentation comments.

Doing a quick test, it seems like nothing shows up within Xcode including quick help, so developers would have to look at the generated sources to see it. For that reason, seems like something added to the doc string might be better.

The deprecation warning only shows for Xcode auto completion when using "future deprecation".

Screenshot 2023-09-11 at 11 05 45 AM
thomasvl commented 1 year ago

Does Xcode give any indication based on that? Or did you mean just because it it is something in the generated source? The comment idea is a good one since it should show up in IDEs with any copied over documentation comments.

Doing a quick test, it seems like nothing shows up within Xcode including quick help, so developers would have to look at the generated sources to see it. For that reason, seems like something added to the doc string might be better.

The deprecation warning only shows for Xcode auto completion when using "future deprecation".

Screenshot 2023-09-11 at 11 05 45 AM

Strange, that wasn't showing up for me in my test.

Sent out the CL for adding it as a comment to start discussion about options.

eseay commented 1 month ago

Seems like it's been a minute since this discussion tailed off, so I'm inclined to ask - can anyone comment on whether or not the limitation still exists?

Additionally, assuming the state of things is still the same, would it be possible/would the limitation still be the same if we were to add "partial support" for a deprecation compiler warning - such that entire messages could be marked as deprecated with an @available annotation, but individual properties would still behave as they do today simply with comments?

thomasvl commented 1 month ago

The problems are when a field is of a type (message or enum) that is marked as deprecated.

There was a recent evolution proposal that starts to make some headway on controlling warnings, but I don't think it will be enough yet to do a better mapping here.

eseay commented 1 month ago

The problems are when a field is of a type (message or enum) that is marked as deprecated.

There was a recent evolution proposal that starts to make some headway on controlling warnings, but I don't think it will be enough yet to do a better mapping here.

Sounds good - thank you. I just wanted to check in to make sure that this was still something that was being pursued and hadn't just become completely stale. Hopefully we'll be able to circumvent this issue eventually. Thanks @thomasvl!

thomasvl commented 1 month ago

We're also open to ideas.

Since the generated code is compiled in other packages, people tend to be sensitive to warnings in those contexts, so that's what makes it hard.

We also don't want a lot of generation options, as that makes maintenance more difficult.