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

[Core] Fix collision with known smithy4s construct names #848

Merged
merged 9 commits into from
Mar 13, 2023

Conversation

yisraelU
Copy link
Contributor

@yisraelU yisraelU commented Mar 3, 2023

reopens #847

yisraelU and others added 2 commits March 3, 2023 13:48
Add logic to prevent collisions shapes generated in other files, which
Scala 3 seems to have trouble with when `"-source:3.0-migration"` is
enabled
 type of implicit definition needs to be given explicitly
import smithy4s.example.refined.Age.provider._
import smithy4s.schema.Schema.int
import smithy4s.schema.Schema.struct

case class StructureWithRefinedMember(otherAge: Option[Age] = None)
case class StructureWithRefinedMember(otherAge: Option[smithy4s.example.refined.Age] = None)
Copy link
Contributor

Choose a reason for hiding this comment

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

What caused this to no longer be imported at the top? I see a few others as well so maybe a regression of some kind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, in f507ab2

Copy link
Contributor

Choose a reason for hiding this comment

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

... guys, this is actually the bug that was fixed, f507ab2 reintroduces it ...

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason why the bug was not exhibited is that the compiler option added was incorrect. See :

f84f208

Copy link
Contributor

Choose a reason for hiding this comment

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

It is normal for the fully qualified name to be rendered here, because the Smithy namespace contains a shape of the same name, and Scala 3 is not happy about it.

Arguably this is a Scala 3 bug, but we can't not workaround on our side.

Now please one of you kindly revert the commit that re-introduced the bug.

Copy link
Contributor Author

@yisraelU yisraelU Mar 10, 2023

Choose a reason for hiding this comment

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

The reason why the bug was not exhibited is that the compiler option added was incorrect. See :

I dont know, I tried that first and got bad option: '-source:3.0-migration'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see this option only works with the Scala 3 compiler

Copy link
Contributor

@Baccata Baccata left a comment

Choose a reason for hiding this comment

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

Good work

@Baccata Baccata merged commit d110f54 into series/0.17 Mar 13, 2023
@Baccata Baccata deleted the fix-collision branch March 13, 2023 11:13
@kubukoz kubukoz mentioned this pull request Mar 15, 2023
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