Skip to content
This repository has been archived by the owner on Apr 18, 2020. It is now read-only.

SBT plugin will most likely fail compile when treating errors as warnings #42

Open
DavidDudson opened this issue Jan 2, 2018 · 6 comments

Comments

@DavidDudson
Copy link
Member

DavidDudson commented Jan 2, 2018

Currently, we do not touch imports in generated files.

When we expand a generator, we remove the annotation.

Thus, code such as

   import foo.bar.Main
  
   @Main object App {
        println("Foo")
    }

will remove the annotation, leaving the import unused.

The solution to this problem is likely hard, but it should be noted somewhere in the docs at the least.

@DavidDudson DavidDudson added the bug label Jan 2, 2018
@tabdulradi
Copy link

Is it possible to configure SBT to treat the generated files differently? (i.e different scalac flags)

@DavidDudson
Copy link
Member Author

No, because then it masks real errors. With a lot of the macro annotations I have seen, the generated code makes up a small portion of the file. You still want compiler warnings for the parts which are untouched.

It would be nice if we could ignore scalac warnings for ranges in documents. I think the eventual approach will be to remove select unused imports though.

@olafurpg
Copy link
Member

olafurpg commented Jan 7, 2018

The original and expanded code should be in separate projects where you can tune the scalacOptions and logger level via sbt. In the generated sources you may want to filter out warnings which can be done with a

      logLevel := Level.Error

You can also override the sbt compilerReporter where you get positions of reported messages, but I don't think that's necessary.

@DavidDudson
Copy link
Member Author

DavidDudson commented Jan 9, 2018

The problem I see is something like

import scala.meta.generators.kase
import cats.NonEmptyList

@kase class Point(x: Int, y: Int)

In the above case:

  1. NonEmptyList is unused
  2. When treating warnings as errors with unused imports enabled it should fail
  3. Otherwise, it should emit a single warning.

After some thought, here's a few ideas I have.

Remove unused imports before compile

Remove all unused imports in file. This would cause compiling when treating warnings as errors to pass.

Cons:

  • would miss the unused import here.
  • Requires scalafix
  • Slower

Selectively remove unused imports only when compiler flag is detected

Remove any unused import of a Generator Subtype.

We only have to do this when detecting the unused imports flag, otherwise, we do not care.

Cons:

  • Requires scalafix and/or a semantic database
  • Slower.
  • Generator imports would not be able to be wildcards or else we may not be able to easily detect th
    em. scala.meta.generators._ would not be removed, for example. (I do don't know enough about the semantic database to know if this restriction could truly be avoided, I presume the semantic database can figure out which imports are used from the wildcard, but have never experimented)

Just do not remove annotations

The only reason we actually get this error is due to removing the annotation, thus leaving the import unused.

Cons:

  • Requires Generator library to be on compile classpath for no real benefit
  • Would have to disable recursion

Force generator authors to use fully qualified names with unused imports

import cats.NonEmptyList

@scala.meta.generators.kase class Point(x: Int, y: Int)

This becomes a non-issue and we do not have to change anything.

Cons:

  • Can we really expect people to do this? It would seem unusable

Note: After some thought, we do not know where the import is, thus we cannot just ignore warnings for this. Even a compiler plugin would not be able to figure it out. So ranges do not help.

To be truly honest, I think this might be the last nail in the coffin for recursion because I like that idea best. Perhaps we should ask the community.

@olafurpg Do you agree?

@DavidDudson DavidDudson added this to the Version 0.1 milestone Jan 9, 2018
@aparo
Copy link

aparo commented Feb 23, 2018

IMHO, the best solutions is to not remove the annotations.
The generator library should not be required to be in the classpath only the annotation declaration is required.
About the recursion, we can mark in the cache which modification is executed so we have an order of already executed ones.

@DavidDudson
Copy link
Member Author

@APari that is the final solution at this stage... nearly complete. We are removing recursion, although you will be able to call other generators from yours, so you an do the same thing as recursion, just without the “discovery” phase.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants