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

Address all errors reported by IntelliJ #3867

Merged
merged 4 commits into from
Dec 13, 2023
Merged

Conversation

Scott-Guest
Copy link
Contributor

@Scott-Guest Scott-Guest commented Dec 11, 2023

This PR addresses all errors reported by IntelliJ.

  • Remove all variadic functions in Constructors.scala and KORE.scala
    • In terms of the generated Java code, a variadic Scala function def foo(x: Bar*)
      • always generates public void foo(Seq<Bar> x)
      • additionally generates public void foo(Bar[] x) when marked with @annotation.varargs
    • Problem: With @annotation.varargs, IntellIiJ is unable to resolve any usage of public void foo(Seq<Bar> x)
      • Dropping @annotation.varargs would fix this, but also lose the public void foo(Bar[] x) version that we use pervasively.
      • Manually declaring the overload public void foo(Seq<Bar> x) fixes the IntelliJ error, but won't compile due to conflicts with the generated one.
    • Solution: Change the signature to def foo(x: Seq[Bar]), then manually provide overloads
      • def foo(x: Array[Bar]) = foo(x.toSeq) to support public void foo(Bar[] x)
      • def foo(x1: Bar, ..., xN: Bar) = foo(Seq(x1, ..., xN)) to support variadic calls with 0 <= N <= 5
  • Move test files to ensure their directory structure actually aligns with their package structure
  • Fix all JavaDoc errors

The only remaining reported error is the use of a deprecated method here:

I was unsure whether this was required for NailGun, and left it in place accordingly.

@Scott-Guest Scott-Guest self-assigned this Dec 11, 2023
@Scott-Guest Scott-Guest marked this pull request as ready for review December 12, 2023 13:57
@radumereuta
Copy link
Contributor

We can discuss during the meeting if we should align our code to IntelliJ or the default compiler.
The IntelliJ analyzer is designed for speed, not necessarily for correctness.
But since IntelliJ is the best way to develop this code, it is nice not to worry about the reported warnings.
So overall I am in favor of these changes.

@Baltoli
Copy link
Contributor

Baltoli commented Dec 12, 2023

Yeah, I'm in favour of making tooling work properly on our code - no point having code that's accepted by the compiler if it's really painful to run our tooling on it because there's a big pile of errors being thrown up.

Copy link
Contributor

@Baltoli Baltoli left a comment

Choose a reason for hiding this comment

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

The n-ary instantiation code is unfortunate, but I'm definitely on the side of having working tooling over beatiful code.

@Baltoli
Copy link
Contributor

Baltoli commented Dec 12, 2023

Perhaps take a look at the Scala version that the IntelliJ checker is using; that might change some of the results. @radumereuta please post settings

@radumereuta
Copy link
Contributor

You may want to look at different checkers in IntelliJ:
image

@Scott-Guest
Copy link
Contributor Author

Scott-Guest commented Dec 12, 2023

@radumereuta @Baltoli Using compiler rather than built-in for error highlighting does seem to address these false errors.

However, I am still in favor of using built-in and merging this PR given that

  • compiler doesn't support many of the IntelliJ niceties like the suggested quick fixes and code quality inspections
  • the cost is low, and the only false error reported by built-in in the whole codebase is the varargs issue described here

@radumereuta
Copy link
Contributor

lgtm, you can merge as is.

@Baltoli
Copy link
Contributor

Baltoli commented Dec 13, 2023

Yep, agreed - go ahead

@rv-jenkins rv-jenkins merged commit aa52e23 into develop Dec 13, 2023
8 checks passed
@rv-jenkins rv-jenkins deleted the fix-intellij-errors branch December 13, 2023 16:02
@Scott-Guest Scott-Guest mentioned this pull request Feb 22, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants