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

(Scala 2) Dependency sources have false-positive diagnostics #4654

Closed
kubukoz opened this issue Nov 19, 2022 · 8 comments
Closed

(Scala 2) Dependency sources have false-positive diagnostics #4654

kubukoz opened this issue Nov 19, 2022 · 8 comments

Comments

@kubukoz
Copy link
Contributor

kubukoz commented Nov 19, 2022

Describe the bug

When you go to dependency sources, you may see INFO diagnostics suggesting there's something wrong.

For example:

//> using scala "2.13.10"
//> using lib "com.disneystreaming.smithy4s::smithy4s-dynamic:0.16.8"
import smithy4s.dynamic.DynamicSchemaIndex

trait A {
  DynamicSchemaIndex
}

Go to DynamicSchemaIndex. You might see this:

image

In this particular case, Schema comes from smithy4s-core. It's definitely in scope and shouldn't be an error/info... but is there any reason we're looking for diagnostics in dependency sources in the first place?

Expected behavior

No diagnostics are emitted.

Operating system

macOS

Editor/Extension

VS Code

Version of Metals

v0.11.9

Extra context or search terms

Some other examples:

org.typelevel::cats-core:2.9.0 -> look at cats.Applicative

image

Apply comes from the same jar so it's not a transitive dependency problem.

it's a bit annoying as it results in a looooooot of fake problems:

image

Even simpler (cats-kernel, which doesn't have transitive dependencies at all and is overall a smaller library):

org.typelevel::cats-kernel:2.9.0 -> look at Order:

image

@kubukoz
Copy link
Contributor Author

kubukoz commented Nov 19, 2022

is there any reason we're looking for diagnostics in dependency sources in the first place?

I guess it's for navigation... which seems to suffer as well, even in Scala 3 where I'm not seeing these diagnostics

@ckipp01
Copy link
Member

ckipp01 commented Nov 20, 2022

Thanks for reporting @kubukoz. I'm trying to reproduce this and it seems a bit non-derterministic for me and I don't really get why. For example with your cats.Applicative example I had no issues with Scala 3, moved to Scala 2.13.10, and then I had issues. The cats.kernel.Order one for me isn't giving me any issues at all actually.

Playing around a bit I keep getting unexpected results, either that or I haven't caught the pattern yet.

I guess it's for navigation

Correct, its a bit different, but we create semanticdb on the fly via the presentation compiler for dependency sources. Something funky is going on in that process its seems.

@tgodzik
Copy link
Contributor

tgodzik commented Nov 21, 2022

I think we might not be adding diagnostics to Semanticdb in Scala 3, which is why these are not being shown. Compiling sources semanticdb might sometimes fail, since we don't have the full build information, so maybe we just shouldn't show the diagnostics in that case?

@ckipp01
Copy link
Member

ckipp01 commented Nov 21, 2022

since we don't have the full build information, so maybe we just shouldn't show the diagnostics in that case?

I do think it's helpful in the situation where we can't to still have some sort of indication that things aren't working. Or else when you try to navigate and it fails it might be confusing.

@tgodzik
Copy link
Contributor

tgodzik commented Nov 21, 2022

since we don't have the full build information, so maybe we just shouldn't show the diagnostics in that case?

I do think it's helpful in the situation where we can't to still have some sort of indication that things aren't working. Or else when you try to navigate and it fails it might be confusing.

Sure, but maybe just have one diagnostic that says the compilation for the file failed and navigation might not work?

@kubukoz
Copy link
Contributor Author

kubukoz commented Nov 21, 2022

since we don't have the full build information

Can libraries overcome this problem by attaching semanticdb to the artifacts they produce? Would Metals use that then?

@tgodzik
Copy link
Contributor

tgodzik commented Nov 21, 2022

It would, but it would require all the libraries to do it and would also make the artifacts larger.

Alternatively, we can just try to make it compile in more scenarios or alternatively make sure that navigation is handled properly even if it fails to compile.

@tgodzik tgodzik added the spree label Nov 30, 2022
@tgodzik tgodzik removed the spree label Dec 30, 2022
@tgodzik tgodzik added the spree label Jun 5, 2023
@tgodzik
Copy link
Contributor

tgodzik commented Dec 7, 2023

Fixed in #5910

@tgodzik tgodzik closed this as completed Dec 7, 2023
@tgodzik tgodzik added this to the Metals v1.1.1 milestone Dec 8, 2023
@tgodzik tgodzik moved this to Done in Metals Issue Board Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants