Skip to content

Avoid passing context and bundle parameters to directives that don't use them. #1168

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

Merged
merged 7 commits into from
Mar 20, 2025

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Mar 6, 2025

Bug/issue #, if applicable: rdar://147405751

Summary

This avoids passing the DocumentationContext and DocumentationBundle to directives and semantic analysis that doesn't use it.

It also deprecates the SemanticAnalysis protocol because it's only conformed to but never used as an existential.

Dependencies

None

Testing

Nothing in particular. This isn't a user-facing change.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • [ ] Added tests
  • Ran the ./bin/test script and it succeeded
  • [ ] Updated documentation if necessary

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@anferbui anferbui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we able to remove the bundle and context parameter requirement from DirectiveConvertible.init?(from:source:for:in:problems)?

I've had a brief look and it looks like it could be possible, but I'm not sure if there's a layer in which they are required.

Otherwise PR looks good.

Comment on lines +51 to +56
@available(*, deprecated, message: "This deprecated API will be removed after 6.2 is released")
extension Semantic.Analyses.ExtractAll: SemanticAnalysis {}
@available(*, deprecated, message: "This deprecated API will be removed after 6.2 is released")
extension Semantic.Analyses.ExtractAllMarkup: SemanticAnalysis {}
@available(*, deprecated, message: "This deprecated API will be removed after 6.2 is released")
extension Semantic.Analyses.HasAtLeastOne: SemanticAnalysis {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mark the whole type (e.g. Semantic.Analyses.HasAtLeastOne) as deprecated, or does it mark just the functions from the protocol that the conformance is being declared on? Never seen @available used in a protocol extension before :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I deprecated the protocol, conforming to it would result in a deprecation warning.

Adding @available(*, deprecated, ...) to the extension that specifies the protocol conformance avoids that deprecation warning without deprecating the entire type (like it would if the conformance was declared in the type definition).

@d-ronnqvist
Copy link
Contributor Author

Are we able to remove the bundle and context parameter requirement from DirectiveConvertible.init?(from:source:for:in:problems)?

I've had a brief look and it looks like it could be possible, but I'm not sure if there's a layer in which they are required.

I'm tackling that in #1175 which builds on the changes from this PR.

There's no directive that uses the context parameter so we can deprecate that and phase it over a couple of releases. Most directives don't use the bundle parameter either but there are a few that access the bundle's identifier to create ResourceReference values.

Copy link
Contributor

@anferbui anferbui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for answering my questions!

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist added the code cleanup Code cleanup *without* user facing changes label Mar 19, 2025
@d-ronnqvist d-ronnqvist merged commit 926d85b into swiftlang:main Mar 20, 2025
2 checks passed
@d-ronnqvist d-ronnqvist deleted the unused-directive-parameters branch March 20, 2025 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Code cleanup *without* user facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants