Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
thomasvl opened this issue Dec 1, 2016 · 19 comments
Open
Labels
kind/enhancement Improvements to existing feature. ⚠️ semver/major Breaks existing public API.

Comments

@thomasvl
Copy link
Collaborator

thomasvl commented Dec 1, 2016

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 thomasvl added ⚠️ semver/major Breaks existing public API. kind/bug Feature doesn't work as expected. labels Dec 1, 2016
@thomasvl thomasvl self-assigned this Dec 6, 2016
@thomasvl
Copy link
Collaborator Author

thomasvl commented Dec 7, 2016

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 thomasvl removed their assignment Dec 7, 2016
@thomasvl
Copy link
Collaborator Author

thomasvl commented Dec 7, 2016

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
Copy link
Collaborator

allevato commented Dec 7, 2016

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 thomasvl added kind/enhancement Improvements to existing feature. and removed kind/bug Feature doesn't work as expected. labels Feb 6, 2017
@thomasvl
Copy link
Collaborator Author

thomasvl commented Feb 6, 2017

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
Copy link

Any update on this or the limitations remain?

@thomasvl
Copy link
Collaborator Author

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
Copy link
Collaborator

tbkka commented Apr 3, 2019

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
Copy link
Collaborator Author

thomasvl commented Apr 3, 2019

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
Copy link

DanF-Go commented Aug 31, 2023

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

@thomasvl
Copy link
Collaborator Author

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
Copy link

DanF-Go commented Sep 6, 2023

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
Copy link
Collaborator Author

thomasvl commented Sep 6, 2023

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
Copy link
Collaborator Author

thomasvl commented Sep 6, 2023

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
Copy link

DanF-Go commented Sep 11, 2023

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
Copy link
Collaborator Author

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.

ahmed-osama-saad pushed a commit to ahmed-osama-saad/swift-protobuf that referenced this issue Oct 12, 2023
Don't pass stackTrace in EventSink.error
@eseay
Copy link

eseay commented Oct 25, 2024

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
Copy link
Collaborator Author

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
Copy link

eseay commented Oct 25, 2024

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature. ⚠️ semver/major Breaks existing public API.
Projects
None yet
Development

No branches or pull requests

6 participants