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

Implement type normalization #15

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

alessionossa
Copy link
Contributor

@alessionossa alessionossa commented Apr 7, 2024

Implement basic type normalization, as discussed in #7.

The Type.normalized() function maps Type elements to their corresponding NormalizedType so that syntax that has the same semantic meaning is mapped to the same syntax under NormalizedType.

The implementation, when it makes sense, tries to keep as much information as possible during the mapping, trivias included.

As a source of truth for what the normalized version should be, I used, where possible, the result obtained by printing the type to the console, for example:

print("array of Int: \([Int].self)")    // array of Int: Array<Int>

and

print("Void: \(Void.self)")    // Void: ()

Closes #7.

@stackotter
Copy link
Owner

Hey Alessio, happy to review this PR but I've just been holding off cause it's still marked as a draft. Just letting you know in case that was an accident 👍

@alessionossa alessionossa marked this pull request as ready for review April 10, 2024 17:26
Copy link
Owner

@stackotter stackotter left a comment

Choose a reason for hiding this comment

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

Looking awesome so far! 🚀 It seems like you've caught most of the edge cases 👌 I've left some comments with a few more edge cases to test out and some aspects of normalization that I think should be changed to make things a bit easier to use or more intuitive. If there's anything that doesn't make sense or you don't agree with, don't hesitate to let me know! Always happy to have a more in-depth discussion.

Sources/MacroToolkit/Type.swift Outdated Show resolved Hide resolved
Sources/MacroToolkit/Type.swift Outdated Show resolved Hide resolved
Sources/MacroToolkit/Type.swift Outdated Show resolved Hide resolved
Sources/MacroToolkit/Type.swift Outdated Show resolved Hide resolved
Sources/MacroToolkit/NormalizedType.swift Outdated Show resolved Hide resolved
Sources/MacroToolkit/Type.swift Outdated Show resolved Hide resolved
return .packReference(.init(type._baseSyntax, attributedSyntax: type._attributedSyntax))
case .simple(let type):
if type.name == "Void" {
return .tuple(.init(.init(elements: [])))
Copy link
Owner

Choose a reason for hiding this comment

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

I think Void is better to have as a simple type as that's the name most people know it by (and using () is considered less swift-y by a few core Swift contributors, even though Void is actually an alias to () under-the-hood).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sources/MacroToolkit/Type.swift Outdated Show resolved Hide resolved
Tests/MacroToolkitTests/MacroToolkitTests.swift Outdated Show resolved Hide resolved
@stackotter
Copy link
Owner

As a source of truth for what the normalized version should be, I used, where possible, the result obtained by printing the type to the console, for example:

...

print("Void: \(Void.self)")    // Void: ()

Hmm that does complicate my suggestion for Void to be normalized to a simple type a bit. I still reckon in this case we should go with Void as the normalized representation since that's what people know it by and what most people prefer in their own code, but being able to use the string representations of types as the source of truth for normalization is a very compelling argument against that idea. I'd love to hear your thoughts.

@alessionossa
Copy link
Contributor Author

Hmm that does complicate my suggestion for Void to be normalized to a simple type a bit. I still reckon in this case we should go with Void as the normalized representation since that's what people know it by and what most people prefer in their own code, but being able to use the string representations of types as the source of truth for normalization is a very compelling argument against that idea. I'd love to hear your thoughts.

I found useful to use string representations of types in my code to avoid misspelling types and keeps cases as type-safe as possible.

In my code using the package I have, for example:

switch (typeName) {
case ("\(Int.self)"):
    getterBody = "integer(forKey: Self.\(identifierTokenSyntax)Key)"
case ("\(Bool.self)"):
    getterBody = "bool(forKey: Self.\(identifierTokenSyntax)Key)"
case ("\([String: Any].self)", true):
    getterBody = "dictionary(forKey: Self.\(identifierTokenSyntax)Key)"

Even considering that Void is generally preferable over () in Swift, we are here trying to normalize types to their most basic form, not trying to make them look nice and swift-y. Also Int? is preferable over Optional<Int>, but Int? it’s not the normalized representation of it.

@stackotter
Copy link
Owner

Even considering that Void is generally preferable over () in Swift, we are here trying to normalize types to their most basic form, not trying to make them look nice and swift-y. Also Int? is preferable over Optional<Int>, but Int? it’s not the normalized representation of it.

Yeah that is a tricky distinction to make. The way that I'm looking at it is that the two concurrent goals for the normalisation are to minimise the number of special cases (i.e. removing specialised Int? syntax etc) and to make the normalised form as predictable as possible (so that developers don't incorrectly assume how something will get normalised).

As I see it, normalisation now has only a single way of representing every type (ignoring the possibility for unnecessary single-element tuple syntax), except for void; macro toolkit has to make a choice between either .simple(.init("Void")) or .tuple(.init(.init(elements: []))).

To me, comparing a type against .simple(.init("Void")) makes it immediately obvious what you're checking for. Whereas it'd be harder to skim-read .tuple(.init(.init(elements: []))) since you'd usually initially assume that it's an actual tuple not () which many people would probably pronounce as 'void' when reading code such as (Int) -> ().

If choosing one option or the other eliminated an enum case from NormalizedType then it'd be a pretty easy choice but unfortunately neither actually simplifies the model so it's more just a question of predictability. I don't think either option's perfect, but I'm still leaning heavily towards Void in terms of how much more readable it is in user code. The only thing that's making me consider () at all is the fact that Swift prints void as ().

I guess if you look at it from the 'reducing the number of special cases' point of view then representing void as "Void" does help reduce the number of special cases of .tuple, because at the moment it can have any number of elements greater than 1, or it can have zero elements. Whereas if void is "Void" then .tuple quite simply just requires more than 1 element.

I'll attempt to do a bit of polling among Swifties that I know and see what people reckon.

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.

Implement type normalization
2 participants