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

allow defining generic type constraints for swift #174

Merged

Conversation

darrell-roberts
Copy link
Member

@darrell-roberts darrell-roberts commented Jun 5, 2024

This updates how generic type constraints are applied to swift types. Before if a type had a generic member we would implicitly apply the same type constraints to the generic constraints. That is not always desirable. This allows the user to define the generic type constraints via the swiftGenericConstraints typeshare attribute.

See example in core/data/tests/generic_struct_with_constraints_and_decorators/input.rs.

Also removed excessive allocations in swift.rs.

Introduced swift config option codablevoid_constraints which allows defining constraints for CodableVoid.

@darrell-roberts darrell-roberts changed the title allow defining generic constraints via #[typeshare(swiftGenericConstr… allow defining generic type constraints for swift Jun 5, 2024
@darrell-roberts darrell-roberts force-pushed the swift-generic-constraints-2 branch from 8165df3 to c5f47d5 Compare June 5, 2024 17:13
Comment on lines 800 to 801
.get(&SupportedLanguage::Swift)
.and_then(|d| d.get(SWIFT_GENERIC_CONSTRAINTS_DECORATOR))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels a bit awkward here (and elsewhere, especially in the kotlin section) to have to first get the values for a specific language, and then drill down into the decorators that match the kind we are looking for. Now that we have multiple decorators for a single language (and we already don't support decorators for every language), would it be unreasonable to decouple the decorator type from the SupportedLanguage struct?

If we had a DecoratorKind enum that included the supported decorators (e.g. Kotlin, Swift, and SwiftGenericConstraints), and then built the map of DecoratorKind => Vec of values, the API would be roughly similar to what it is now, but allowing multiple kinds of decorators to be pulled when they are relevant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I agree. Pushed the update.

@darrell-roberts darrell-roberts force-pushed the swift-generic-constraints-2 branch from 20e3b98 to 17535fa Compare June 5, 2024 21:39
@darrell-roberts darrell-roberts force-pushed the swift-generic-constraints-2 branch from 17535fa to cc894cd Compare June 5, 2024 21:40
@darrell-roberts darrell-roberts force-pushed the swift-generic-constraints-2 branch from 18a516c to 42c9e70 Compare June 6, 2024 01:35
@darrell-roberts darrell-roberts force-pushed the swift-generic-constraints-2 branch from fe5baed to cee3d3b Compare June 6, 2024 13:06
@darrell-roberts darrell-roberts force-pushed the swift-generic-constraints-2 branch 2 times, most recently from 6a9911a to d24c1c9 Compare June 6, 2024 13:34
@darrell-roberts darrell-roberts force-pushed the swift-generic-constraints-2 branch from d24c1c9 to b9a22ae Compare June 6, 2024 15:01
Copy link
Collaborator

@charlespierce charlespierce left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Left one suggestion for cleaning up the need to pass Cow into a helper, but that's not blocking. I'm still waiting on some more info about the release process, so I'll look to merge this in the morning. If you get to making that change by then, great; if not, happy to have it as a follow-up PR since it doesn't really affect the working of the module, just the dev ergonomics.

use crate::visitors::accept_type;

/// Type level typeshare attributes are mapped by target language and a mapping of attribute.
pub type DecoratorMap = HashMap<DecoratorKind, BTreeSet<String>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love using a BTreeSet here instead of sorting & deduping as part of importing 🎉

Comment on lines 856 to 860
fn swift_keyword_aware_rename(name: Cow<str>) -> Cow<str> {
if SWIFT_KEYWORDS.contains(&name.as_ref()) {
Cow::Owned(format!("`{name}`"))
} else {
name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Returning Cow<str> here makes a lot of sense, however needing to pass one in strikes me as a bit of an anti-pattern. It looks like that might be used to avoid lifetime issues with the format! calls above and "value dropped while borrowed" or something? If so, would it be reasonable to take an Into<Cow> to avoid needing to update all of the call sites? I'm thinking something like:

fn swift_keyword_aware_rename<'a, T>(name: T) -> Cow<'a, str>
where
    T: Into<Cow<'a, str>>,
{
    let name = name.into();
    if SWIFT_KEYWORDS.contains(&name.as_ref()) {
        Cow::Owned(format!("`{name}`")
    } else {
        name
    }
}

That way we still avoid the extra allocations, but we can directly pass either a String or &str into the method without needing to explicitly annotate Cow::Owned or Cow::Borrowed at the call site.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup I felt I missed something when repeatedly wrapping in Cow. Pushed update.

Comment on lines +278 to +287
Either::Left(
self.get_default_decorators().chain(
swift_decs
.iter()
.filter(|d| d.as_str() != CODABLE)
.map(|s| s.as_str()),
),
)
} else {
Either::Right(self.get_default_decorators())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a nice pattern! I don't think I've seen Either used like this for the two branches to allow a simple iterator fold at the end, but it's a lot more readable than collecting into an intermediate vector.

@charlespierce charlespierce merged commit a03f5ef into 1Password:main Jun 6, 2024
5 checks passed
@darrell-roberts darrell-roberts deleted the swift-generic-constraints-2 branch June 7, 2024 12:21
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.

2 participants