scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.85k stars 1.06k forks source link

Incremental compilation does not work if modified code is imported from a module that exported said code from elsewhere. #18216

Open phrmoy opened 1 year ago

phrmoy commented 1 year ago

Compiler version

3.3.0

Minimized code

ModuleA.scala

object ModuleA {
  def func(x:Int) = x
}

ModuleB.scala

object ModuleB {
  export ModuleA.{*}
}

ModuleC.scala

object ModuleC {
  import ModuleB.{*}
  println(func(0))
}

The above works. Now if I change the ModuleA.scala to

ModuleA.scala

object ModuleA {
  def func(x:Int, y:Int) = x + y
}

and change ModuleC.scala to

ModuleC.scala

object ModuleC {
  import ModuleB.{*}
  println(func(0,0))
}

Both SBT and bloop fail to compile the code.

Bloop v1.5.6. SBT v1.8.3

Output

From SBT

[error] 11 |  println(func(0, 0))
[error]    |               ^^^^
[error]    |               Found:    (Int, Int)
[error]    |               Required: Int
[error]    |----------------------------------------------------------------------------
[error]    | Explanation (enabled by `-explain`)
[error]    |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

From Bloop

Found:    (Int, Int)
Required: Int

Expectation

Expected code to compile normally.

phrmoy commented 1 year ago

Howdy, how goes? For the lack of not understanding any better when and how open issues are handled, would it be ok for me to ask what would be an expected/average ETA for something like this to be fixed? Currently, I am still relying heavily on exports, and not being able to refactor (rename methods) without having to delete entire builds is really hurting development. Unfortunately, I cannot offer anything "more" at the moment, so I am merely asking. If the answer is "not planned" and/or "unkown" I understand, at least I will be able to plan accordingly. Thank you.

bishabosha commented 1 year ago

would it be ok for me to ask what would be an expected/average ETA for something like this to be fixed?

in general when prioritising bugs there is usually two considerations - did this break something that worked in the past, or is it blocking a large enough project.

Failing those it goes to asking how common the pattern is, so then if only 1 person is reporting it, then it would seem to not be a large issue (however i'm sure many people experience bugs but stay quiet for whatever reasons).

Personally, I assigned myself because I know the code, I can give a look in the morning, but otherwise if someone reads this and thinks they want to try a fix then don't hesitate to respond here and ill unassign myself

phrmoy commented 1 year ago

Thank you for your reply. I will leave some thoughts and a question.

in general when prioritising bugs there is usually two considerations - did this break something that worked in the past, or is it blocking a large enough project.

As far as the "defect" I am encountering, export works as expected "when it compiles." The problem is to get it to compile when you refactor. Thus it should not be breaking projects (if the code compiles).

Failing those it goes to asking how common the pattern is, so then if only 1 person is reporting it, then it would seem to not be a large issue (however i'm sure many people experience bugs but stay quiet for whatever reasons).

Despite the number of reports, for this particular case, one can reason to the conclusion that it impacts everyone who uses export in that if they refactor code, it makes the compiler throw false positives. That said, export works when it compiles. However, particularly for the people following the recommended Metals setup, it involves manually deleting the bloop folder to get the compiler to accept the refactored code. This is enough reason not to use export in the code, which one could argue next that it deems the export feature unusable. If this feature is not experimental, this should be under consideration. Sadly, I find export extremely useful (this sentiment, however, I cannot say if it's shared at large).

Now on to the question, would it be ok for me to reach out to the forum and raise awareness on this issue?

bishabosha commented 1 year ago

I can't reproduce the problem you are having from the snippets provided, you will need to be more specific about when each compilation occurs, and when each change occurs

steps I took:

som-snytt commented 1 year ago

would it be ok for me to reach out to the forum

That is a fine idea. A more public conversation about an issue might result in shared workarounds or other experience with a feature.

phrmoy commented 1 year ago

I have attached a short clip that at least shows the issue.

https://youtu.be/y54_IWrGQh8

bishabosha commented 1 year ago

I have attached a short clip that at least shows the issue.

https://youtu.be/y54_IWrGQh8

could you explain why there appears to be an implicit argument to export ModuleCUtils(x$1).{*}?

I still couldn't replicate this, (using same metals snapshot too) maybe you can create a GitHub project?

phrmoy commented 1 year ago

Thanks for looking into this.

I believe that implicit argument, in the video, seems to be coming from inside ModuleCUtils.

  opaque type ModuleCUtilsType = Object
  given ModuleCUtilsType = Object()
  def validateModuleCUtilsImports(using ModuleCUtilsType) = ()
bishabosha commented 1 year ago

yes that would makes sense, because the invisible exported forwarder for validateModuleCUtilsImports would need to pass the implicit argument (but given its invisible that makes it a bit awkward)

phrmoy commented 1 year ago

Clearly, the example I shared is a simple toy example. Unfortunately, I am encountering this issue in my real code base time and again, and it's really hindering development to a point where I am not sure what to do next, seeing I have already invested months in this Scala code base. Not a threat of any kind, just feeling a little hopeless with this situation, and being a decision maker, I need to consider all options. As it is, if we cannot reproduce the bug on a toy problem, I am not sure how I would figure out the code base that has grown long and complex.

It pains me to know that this is probably not a tooling thing but rather a compiler thing since both Bloop and SBT throw the false positive.

I know you asked me to share a GitHub repo, I will work on that as soon as time allows.

bishabosha commented 1 year ago

it sounds like you are in a team, you can confirm that most/all people have the same problem?

bishabosha commented 1 year ago

it might be also good to get a log of the differences sbt detects, e.g. as detailed in the sbt docs

raquo commented 10 months ago

@bishabosha I finally managed to make a small and seemingly reliable reproduction for what I think is the same issue.

(I'm facing a very similar looking problem in real life with JsRouter in https://github.com/raquo/laminar-full-stack-demo/ and a private project that uses a similar pattern.)

  1. Unzip inctext.zip (it's a small project made from sbt new scala/scala3.g8 as you suggested)
  2. Open the Page.scala and View.scala files in a simple text editor – NOT AN IDE – because you will need to manually control the timing of saving the files, and you don't want the IDE to touch sbt.
  3. Open sbt. clean then compile then ~compile, wait for it to finish
  4. In View.scala, uncomment navigateTo(FooPage(1)). Save View.scala now
  5. sbt will rightly complain that FooPage does not exist
  6. In Page.scala, uncomment the line defining FooPage. Save Page.scala file now
  7. sbt will again complain that FooPage does not exist, but it does exist now
  8. Press enter in sbt to exit ~compile, and run the compile command – same error persists
  9. Do clean and compile - the error goes away.

ETA: Slightly rearranged instructions for clarity.

Disclosure: Originally I reproduced this by typing out the commented code but I think it should work with a comment too. I have a video if needed, but it's pretty reliable, I hope you can readily reproduce this. I think saving an invalid View.scala first is the key to the reproduction.

bishabosha commented 9 months ago

@raquo thank you for the detailed analysis, I was able to reproduce your steps!