facebook / buck

A fast build system that encourages the creation of small, reusable modules over a variety of platforms and languages.
https://buck.build
Apache License 2.0
8.56k stars 1.16k forks source link

Project generates incorrect IntelliJ source roots for Kotlin modules #1612

Open cwoodwar6 opened 6 years ago

cwoodwar6 commented 6 years ago

After running buck project on a project that has kotlin (a project with rules such as kotlin_library, android_library with language set to Kotlin, etc), the source roots of those modules are not made correctly in IntelliJ.

Here are a couple examples: image Note how the app directory is marked as the source root instead of src/main/java which is where the packages actually begin.

image And here, the kotlin-android module is the source root instead of src/main/java

Both of these examples could be src/main/kotlin depending on your naming conventions and/or use of mixed sources.

This causes the IDE to behave incorrectly when refactoring, importing those classes, and even when defining the package of that module.

If any of these modules are turned into pure java modules, they look correct: image

styurin commented 6 years ago

Check this PR: https://github.com/facebook/buck/pull/1417

buck project doesn't have a good support for Kotlin files. To properly understand package information from Kotlink files we need to add parsing to buck project in the same way we do for Java files.

kageiit commented 6 years ago

More info on https://github.com/facebook/buck/commit/263b819665078c52f8368d1a12a53f17c640c18d from @asp2insp reg. why this does not work currently and how it can be made to work

asp2insp commented 6 years ago

Thanks @kageiit ! Mirroring my comments here:

I think the list of necessary changes are:

  1. Adding correct sourcefile detection
  2. Switching the language support module rules to opt in:
  3. Change this regex here to add other "end of prelude" keywords from those languages, e.g. fun|def
  4. (This is the tricky part) We need some equivalent functionality to JavaFileParser#getPackageNameFromSource for each of these languages. This would create an AST and walk it to find the package declaration (this removes false positives such as the ones in the test cases contained in this patch)

    I'd be happy to code/review or provide more pointers if someone wanted to take this on.

kageiit commented 6 years ago

Seems like antlr would be a good candidate to perform simple ast parsing to get just the packages for all these languages (and potential future languages as well)

The antlr core runtime is quite small (300k) - https://mvnrepository.com/artifact/org.antlr/antlr4-runtime/4.7.1

Parsers exist for all three languages and can be compiled into a small runtime Groovy - https://github.com/groovy/groovy-core/blob/master/src/main/org/codehaus/groovy/antlr/groovy.g Kotlin - https://github.com/antlr/grammars-v4/tree/master/kotlin Scala - https://github.com/antlr/grammars-v4/tree/master/scala

@asp2insp would you be ok if there was a PR with this approach?

asp2insp commented 6 years ago

I like that approach, but I'd also like to get input from @styurin or @ttsugriy since this involves adding new dependencies :)

christiandeange commented 6 years ago

For those looking for a quick fix, I've written up a python script that post-processes .iml files after buck project has generated them, and touches up the folder definitions to correct them. We use this internally at Square and it seems to cover all the use cases we have for misconfigured modules.

If you're using okbuck, append this to your buck wrapper after running BUCK_BINARY to invoke the script automatically:

if [[ $? == 0 && "project" == $1 ]]; then
    python $SCRIPT_DIR/scripts/fix_sources.py -p $SCRIPT_DIR
fi
kageiit commented 6 years ago

Alternatively, one can apply this patch to buck to have buck project respect source roots correctly without having to post process. Its a similar approach, but happens inside the project command itself

https://gist.github.com/kageiit/4e48964cb6d9382737b96c21e1adf730

kageiit commented 6 years ago

cc @cdeange @cwoodwar6 ^