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

Check for explicit result types in public API #2922

Merged
merged 8 commits into from
Jan 14, 2024
Merged

Conversation

lefou
Copy link
Member

@lefou lefou commented Dec 13, 2023

So, I don't have much experience with scalafix. I'd expect that this change should detect / modify a lot of sources where we have missing type annotations, but it doesn't. Any help is welcome.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 13, 2023

is it just --check that doesnt work, but fix does? If so we could go back to the earler git diff way of checking and failing in CI

@lefou
Copy link
Member Author

lefou commented Dec 13, 2023

is it just --check that doesnt work, but fix does? If so we could go back to the earler git diff way of checking and failing in CI

Unfortunately not. A pure __.fix does change exactly nothing, but I'm pretty sure we still have a lot of public defs without explicit return types.

@ckipp01
Copy link
Collaborator

ckipp01 commented Dec 14, 2023

So this is a bit random, but I dug into this a bit since when trying to run a scalafix rule from within Metals it also fails with a TextDocument.uri not found, which doesn't make a ton of sense. For example I have a very minimal hello world project with a build like this:

import mill._, scalalib._

object millTest extends ScalaModule {
  def scalaVersion = "2.13.12"
}

And then I do have a milltest/src/example/Hello.scala. The error message I was getting from Scalafix was

SEVERE: Internal error: scala.meta.internal.metals.ScalafixProvider$ScalafixRunException: TextDocument.uri not found: milltest/src/example/Hello.scala
java.util.concurrent.CompletionException: scala.meta.internal.metals.ScalafixProvider$ScalafixRunException: TextDocument.uri not found: milltest/src/example/Hello.scala

This comes from here in Scalafix. It was totally by chance that it then clicked.

There is a mistake here that I made that Mill doesn't care about. My module name is millTest, but my directory structure is milltest. Therefore Scalafix will do the lookup for the text document at META-INF/semanticdb/milltest since that's the file structure, but it's actually at META-INF/semanticdb/millTest to match the module name. This causes scalafix to puke and not find a document for the file. I can reproduce this in a minimal hello world project, but my guess would be that in your Mill build you may also have a directory structure with names that maybe differ from the module name? The same thing could be happening here.

@lefou
Copy link
Member Author

lefou commented Dec 14, 2023

@ckipp01 Thanks for commenting. From looking at all the semanticdb file paths, I can't detect any misfitting path names. Also, shouldn't the error message pop up somewhere in that case anyways?

Just in case you're suspicios (like me) whether it could be related to how Mill generates semanticdb data, I have to say, I tried out mill-scalafix a long time ago (maybe 2-3 years?), even before it started to use Mill's semanticDbData target, and had a very same experience. Nothing seemed to work, so I never gave much attention to all the scalafix news. (I think I even tried to use scalafix when migrating Mill 0.6.x to 0.7.x, from Scala 2.12 to 2.13, but it was fruitless to me.) As all of this is on Mill repos, there might be other hidden assumptions in scalafix?

@lefou
Copy link
Member Author

lefou commented Dec 14, 2023

I guess, I'll open an issue in mill-scalafix as well.

@lefou lefou added the upstream The issue originates in an upstream dependency label Dec 14, 2023
lefou added a commit to lefou/mill-scalafix that referenced this pull request Dec 14, 2023
This reproduces an issue discovered in a Mill PR

* com-lihaoyi/mill#2922
@tgodzik
Copy link

tgodzik commented Dec 14, 2023

Manually running Scalafix in Metals looks to be working.

mill

@lefou
Copy link
Member Author

lefou commented Dec 14, 2023

I'm able to reproduce the issue in a mill-scalafix integration test, so this seems to be an upsteam bug.

lefou added a commit to lefou/mill-scalafix that referenced this pull request Jan 6, 2024
This reproduces an issue discovered in a Mill PR

* com-lihaoyi/mill#2922
@lefou lefou removed the help wanted label Jan 7, 2024
joan38 pushed a commit to joan38/mill-scalafix that referenced this pull request Jan 7, 2024
This reproduces an issue discovered in a Mill PR

* com-lihaoyi/mill#2922
@lefou lefou force-pushed the lint-explicit-result-types branch from c033120 to 00cc1fd Compare January 8, 2024 15:52
@lefou lefou removed the upstream The issue originates in an upstream dependency label Jan 8, 2024
@lefou lefou changed the base branch from main to add-result-types January 8, 2024 17:24
Base automatically changed from add-result-types to main January 11, 2024 13:49
@lefou lefou marked this pull request as ready for review January 14, 2024 16:34
@lefou lefou merged commit 21806c1 into main Jan 14, 2024
37 of 38 checks passed
@lefou lefou deleted the lint-explicit-result-types branch January 14, 2024 16:35
@lefou lefou added this to the 0.11.7 milestone Jan 14, 2024
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.

4 participants