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

Add warnings for private access on traits #1508

Merged

Conversation

milesziemer
Copy link
Contributor

Previously, it was possible to refer to a trait marked with @private from another namespace. This commit makes doing this cause a warning. A warning is used instead of an error to maintain backward compatibility, but it will be upgraded to an error in the future.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Previously, it was possible to refer to a trait marked with `@private`
from another namespace. This commit makes doing this cause a warning.
A warning is used instead of an error to maintain backward compatibility,
but it will be upgraded to an error in the future.
@milesziemer
Copy link
Contributor Author

I want to try adding a check to see if the invalid trait relationship is via a mixin. That way we can suggest adding localTraits on the mixin.

@milesziemer
Copy link
Contributor Author

milesziemer commented Nov 28, 2022

Take this model for example:

namespace example.private

// Mixin with the private trait
@private
@mixin
structure PrivateMixin {}

// Trait uses mixin, inheriting the private trait
@trait
structure someTrait with [PrivateMixin] {}
namespace example.main

use example.private#someTrait

// Using the trait from another namespace
@someTrait
structure ExampleStructure {}

The warning message this PR would produce is:

[WARNING] example.main#ExampleStructure: This shape has an invalid trait relationship that targets a private shape, `example.private#someTrait`, in another namespace.

The issue I see here is that it isn't clear how someTrait is getting the @private trait. In a simple example like this, it's not a big deal, but in a more complex model it could be much more confusing.

Do we want to try to handle this case in this PR, or open a separate issue?

After discussion offline, this issue is out of scope for this PR, so it has been moved out of draft.

@milesziemer milesziemer marked this pull request as ready for review November 28, 2022 21:45
@milesziemer milesziemer requested a review from a team as a code owner November 28, 2022 21:45
@milesziemer milesziemer merged commit 9f393b6 into smithy-lang:main Nov 29, 2022
@milesziemer milesziemer deleted the add-private-trait-access-warnings branch November 29, 2022 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants