HaxeFoundation / intellij-haxe

Haxe plugin for IntelliJ Platform based IDEs (IDEA, Android-Studio)
http://intellij-haxe.org
Apache License 2.0
222 stars 100 forks source link

Better/easier management of compiler conditional (#if...#end) definitions. #622

Open EricBishton opened 7 years ago

EricBishton commented 7 years ago

When code is protected by compiler conditionals, (#if...#elseif...#else...#end,) the code that is not on the current compile path is colorized to show that it is not being used. However, figuring out all of the variables that need to be set and managing them is not an easy task: they have to be deduced by the user and entered into the Project Structure->Module->Haxe->Project Definitions list by hand. This is error prone (definitions are easily missed) and tedious, particularly for large projects with multiple targets. We should extend the functionality in the following way(s):

1) Parse the project files (.xml, .hxml, .nmml) from the current target compiler (HXML, NMML, HaxeCompiler, OpenFL), looking for definitions that can be set automatically. 2) Set the definitions according to what the compiler would set during a compile run (possibly as a side effect of other definitions). 3) Be able to set variables based upon the included project libraries. (e.g. myhaxelib="3.1.1"). (Note: These are normally set via haxelib path which sets the lib version and whatever is included in extraParams.hxml. haxelib path is called from the compiler when it sees a -lib argument.) 3) If the above project files contain multiple configurations, be able to pick/use one of them. 4) Be able to toggle the activation/deactivation of individual definitions, overriding the above.

The definitions will be applied in the following order (last having precedence): 1) Given the project's target platform(flash, nme, linux), pick the variables as the compiler would. 2) Given the project's included libraries, set the variables as the compiler would. 3) Parse the project file, selecting a single configuration if more than one is specified. 4) Add user-defined definitions fro the Project Structure->Modules->Haxe->Project Macros sub-dialog.

Things needed to accomplish this: 1) Parse the project files. (We can already do this, we just need to call it for these purposes.) 2) Parse the compiler arguments for -D and -lib from Project Structure->Modules->Haxe->XXX Arguments' 3) Call haxelib path with the appropriate arguments. 4) A way to detect multiple configurations, and a way to select one of them to be current. 5) Optionally, a way to set individual definitions without going through the Project Settings dialog.

Proposed UI: 1) Update the Project Structure dialog to show the "current" set of configured variables. Where variables are defined should be shown somehow, and those that are provided via libs or project files should be colored differently from those the user has overridden/added directly. 2) Add a method to select a configuration to the Project Structure dialog. 3) Add some right-click menu items (or possibly intentions):

mayakwd commented 6 years ago

Well this is topmost annoying "feature" for our developers. Lot of our code are building by gradle-tasks, so solutions with macro-definitions, parsing "-D" variables and so on - becoming useless. Most part of developers perceive this feature as bug.

I think we should revert this solution, and as alternative solution add folding for unnecessary code. Current if-def management are breaking usages finding, refactoring and even "Find in Path".

EricBishton commented 6 years ago

This feature fixes many bugs which have as their root the inability to parse code constructs that become invalid when multiple paths of a compiler conditional are perceived as valid code. A number of the improvements in parsing and resolution (and Find Usages!) that appeared in the June 0.11.0 release are directly due to this feature being implemented (correctly). For that reason, simply reverting this code is a non-starter.

Let's have a discussion about why this is annoying:

I know very little about gradle, and I'm sure saying "we use gradle" is like saying "we use make." It's not that you use a tool, it's how you use that tool that is important -- and can be the cause of friction. Even if I knew gradle well I doubt I could make any reasonable deductions about the use cases that are giving you trouble. If you take the time to write up something that tells me how you use gradle and describe the problems in detail, then I will spend the time to understand them and come up with a solution.

EricBishton commented 6 years ago

BTW, code folding only hides the code from the user (or simplifies their view of it). It doesn't affect the contents of the file or how the file is parsed.

mayakwd commented 6 years ago

Is managing the definitions hard? There are plans to make it easier by reading the project files and library metadata.

Definitely. The main reason why is that annoying - we have to define a lot of macro variables, and try to switch between them in non-intuitive way, process of "re-parsing" taking a lot of time in our projects (they are very huge), it's not 1-2 seconds, it's a 10+ seconds. Imagine that you are debugging code related to two platforms, where additionally exists if-def for the debug/release versions, and a few more macro-definitions for branching to different assemblies.

Or imagine that you set "html, flash" definitions. And you have code like:

#if flash
trace('flash');
#elseif html
trace('html');
#else 
trace('something else');
#end

Are there bugs in refactoring because of this? First I've heard of it.

package ;
class Test {
    public function new() {
        #if flash
        hello("flash");
        #else
        hello("not flash");
        #end
    }

    private function hello(s:String) {
        trace(s);
    }
}

Try to rename hello method.

Is FindUsages broken? How? FindUsages will treat non-available code paths as comments, so the usages will show up there.

Try to find usages of hello method

How is "Find in Path" broken because of this?

Okay, it's not broken, it became unusable with default dark theme

Is the code more ugly on screen if it's colored like a comment? We can easily fold the sections that are the "false" blocks. Is that good enough?

No it's not - best variant if it's being parsed as normal code, we should find a way to make it works.

Would the ability to "turn this off" from a UI configuration be helpful? Then, at least, you can select which bugs bother you more.

We need to rework this mechanism to normaly parse all of the code, and just let to fast fold/unfold code between chosen definitions, that's my point.

I know very little about gradle, and I'm sure saying "we use gradle" is like saying "we use make." ...

You could perceive that statement like that we are not using compilation service proposed by plugin, because build steps have complexity, that plugin can't cover. So plugin mustn't rely on compilation settings within syntax parsing and annotation (at least it should be optional).

EricBishton commented 6 years ago

Just so you know: I am all for making this easier. (I wrote this issue, after all.) And I would love to make it so that all paths can be highlighted, refactored, etc. at the same time.

And know this: This problem is extremely complex, and AFAIK has never been solved in another IDE. They all take the same approach the plugin currently does -- to select one possibility and evaluate only the conditional blocks that become "true."

The crux of the problem is that there is a combinatorial explosion of possibilities that change with every definition (and every value of that definition). There is no way to compute them all in a timely fashion, because you have to re-parse the file for each possibility. And to do complex tasks like refactoring, you effectively have to determine the effect on every (viable) combination before you can decide to do it. (And, then, how do you effectively display that refactoring to the user?)

we have to define a lot of macro variables, and try to switch between them in non-intuitive way, process of "re-parsing" taking a lot of time in our projects (they are very huge), it's not 1-2 seconds, it's a 10+ seconds.

Processing time is expensive. I get that. And waiting for 10+ seconds is bad (believe me, the TiVo code base is more like 30 seconds). I think that re-processing should be done in the background. However, the way that IDEA deals with it (outside of the control of the plugin, unfortunately) forces the re-parsing on the foreground task.