disneystreaming / smithy4s

https://disneystreaming.github.io/smithy4s/
Other
340 stars 68 forks source link

scala FQNs may get mistaken for a relative namespace #1345

Open kubukoz opened 6 months ago

kubukoz commented 6 months ago

Consider this smithy:

$version: "2"

namespace hello

long Long

It works fine. However, if you add another file:

$version: "2"

namespace hello.scala

structure NotThatLong {}

then the first one's Scala no longer compiles:

[error] -- [E008] Not Found Error: /Users/kubukoz/projects/demos/target/scala-3.3.1/src_managed/main/scala/hello/package.scala:3:25 
[error] 3 |  type Long = hello.Long.Type
[error]   |              ^^^^^^^^^^^^^^^
[error]   |              type Type is not a member of object hello.Long.
[error]   |              Extension methods were tried, but the search failed with:
[error]   |
[error]   |                  Cyclic reference involving val schema
[error] -- [E008] Not Found Error: /Users/kubukoz/projects/demos/target/scala-3.3.1/src_managed/main/scala/hello/Long.scala:10:34 
[error] 10 |object Long extends Newtype[scala.Long] {
[error]    |                            ^^^^^^^^^^
[error]    |                            type Long is not a member of hello.scala
[error] -- [E008] Not Found Error: /Users/kubukoz/projects/demos/target/scala-3.3.1/src_managed/main/scala/hello/Long.scala:15:37 
[error] 15 |  val underlyingSchema: Schema[scala.Long] = long.withId(id).addHints(hints)
[error]    |                               ^^^^^^^^^^
[error]    |                               type Long is not a member of hello.scala

it actually gets easier to understand if both types are named Long:

- structure NotThatLong {}
+ structure Long {}

because then the message says:

[error] -- [E007] Type Mismatch Error: /Users/kubukoz/projects/demos/target/scala-3.3.1/src_managed/main/scala/hello/Long.scala:15:69 
[error] 15 |  val underlyingSchema: Schema[scala.Long] = long.withId(id).addHints(hints)
[error]    |                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |                     Found:    smithy4s.schema.Schema[Long]
[error]    |                     Required: smithy4s.schema.Schema[hello.scala.Long²]
[error]    |
[error]    |                     where:    Long  is a class in package scala
[error]    |                               Long² is a class in package hello.scala²

I think this would have to be solved by referring to _root_.scala.Long.

BTW I swear I didn't make this case up, I was just trying to generate code for BSP 😅 Long actually exists, and so does the relative .scala namespace.

kubukoz commented 6 months ago

uh, I tried prefixing everything with _root_ and it's quite a bit:

plus, I could've missed a string or two because they're kinda all over the place (which we could actually centralize as part of this fix)

before I go and commit to this, I'll need to know if we're all fine with such a solution, or whether we want to do anything smarter than that.

I would suggest that we stick to this - if we were to e.g. only prefix _root_ when there's a conflict, I'm pretty sure we'd have to know about not only all the present namespaces, but also the entire compile-time classpath of the module we're generating in. And that's very build-tool-specific.

Baccata commented 6 months ago

I'll need to know if we're all fine with such a solution, or whether we want to do anything smarter than that.

Here's what I think :

  1. We should make it an opt-in, as it pollutes the generated code and it's a pretty edge case.
  2. We can centralise the _root_ prepending to here, provided we amend the rendering of hints to also use NameRef.
kubukoz commented 6 months ago

Sounds good. Should it be configured by smithy metadata?

Baccata commented 6 months ago

@kubukoz yeah preferably so

kubukoz commented 1 month ago

Possible (temporary?) workaround: rename the shapes before codegenning.

it'd still be good to implement this though