KT-4569: Intention to simplify boolean expressions with constants

[wutalman]

Intention to simplify boolean expressions with constants in them,
issue: KT-4569 (http://youtrack.jetbrains.com/issue/KT-4569)

I used some helper functions, I didn't know if the convention is to nest them inside the functions they're helping or not but when I looked at some other code it seemed that we just have them as private functions in the class so that's what I did

Parentheses handling:
if the binary expression has parentheses in it, and they don't contain any constants, they are kept as to not change what the user wanted to write

• example: false || x || (y && z) -> x || (y && z)

if the parentheses have constants in them and so the inner expression is changed, I will remove the parentheses only if there's only one element left: (that is again to keep the user's original intent)

• example 1: x || (y && true) -> x || y
• example 2: x || (y && z && true) -> x || (y && z)

if at the end of the evaluation the entire expression has parentheses around it, they will be removed:

• example: false || (x && y) -> x && y

[svtk]

Hi! Sorry for not so fast review =).
Let's improve this functionality by covering not only "true" and "false" constants, but any constant expressions that can be reduced to "true" or "false" as well, like "1 < 2".
This can be done automatically for you by our ConstantExpressionEvaluator, and I've extracted the method "canBeReducedToBooleanConstant" in CompileTimeConstantUtils (you have to update to get it).
It needs the trace, the best way to get is

    val bindingContext = AnalyzerFacadeWithCache.getContextForElement(expression)

[wutalman]

Hey, I'm having some problems with getting canBeReducedToBooleanConstant to work correctly, so far the only thing that it evaluates is true or false, here is the test function i've been using for it:

private fun JetExpression.canBeReducedToTrue(): Boolean { val bindingContext = AnalyzerFacadeWithCache.getContextForElement(this) val trace = DelegatingBindingTrace(bindingContext, "trace for constant check") val res = CompileTimeConstantUtils.canBeReducedToBooleanConstant(this, trace, true) println(this.getText() + " can be reduced to true? " + res) return res }

fun trueReturner(): Boolean = true


and when i test it i get these results:

trueReturner() can be reduced to true? false
2 > 1 can be reduced to true? false
true can be reduced to true? true

any idea where i'm going wrong ?
thanks!

[svtk]

I've by accident left the check for JetConstantExpression in 'canBeReducedToBooleanConstant'.
So the first check

    if (!(expression instanceof JetConstantExpression) || expression.getNode().getElementType() != JetNodeTypes.BOOLEAN_CONSTANT) {
        return false;
    }

should be just

    if (expression == null) return false;

Please add this change to your pull request. Thank you.

[wutalman]

Hey, I'm having trouble with the behavior of canBeReducedToBooleanConstant
if i have something like this
val x = 2
val y = 3
if ( y > x ) { ... }

it will turn it into
val x = 2
val y = 3
if ( true ) { ... }
but i don't think that behavior is correct, while vals can't be reassigned the user can change the text in them sometime and then the condition will remain true and might cause problems.
what do i do about that ?
thanks

[NataliaUkhorskaya]

Hi,
You are right that such expressions shouldn't be reduced to constant.
But to make it works, you should refactor ConstantExpressionEvaluator, because now it computes a constant for all constant expressions (including final properties).
First of all you should add a flag in CompileTimeConstant which will mean that this value use variables as a constant values. There are already some flags (like isPure or canBeUsedInAnnotations), so it will be great if you combine them into one int field.
Then you should write this flag in ConstantExpressionEvaluator when it creates a compileTimeConstant for expression.
And finally you can use only expresions with constants with this new flag when you check is your intention applicable or not.
It isn't an easy task, so feel free to ask any questions you'll have.

[geevee]

Side note: please write meaningful commit messages, e. g.:

KT-4569 Intention to simplify booleans expression with constants

 #KT-4569 fixed

And commits like "fixes" should be squashed into original one at the end of review. Thank you.

[wutalman]

umm okay, it looks like i've screwed up here, i realized this PR was opened a while ago and i wanted to update my stuff so i pulled and rebased into this branch, and it turns out that everything i pulled got into this PR.
I'm not sure what to do in this situation, should i open a new one with the new repo state instead ?
sorry

on a programming related note:
I see the isPure flag exists only on IntegerValue and on those inheriting from it but not on the abstract CompileTimeConstant, should the new flag exist on the abstract class ?
would the new flag basically say whether or not this compileTimeValue is referenced from by a name or not ? (like set it to true in the visitor for SimpleNameExpression but false in the visitor for ConstantExpression ?)
thanks!

[geevee]

Seems that you rebased master onto your branch, instead of the opposite. No need in opening new pull requests. You can use force push to fix it.

• Checkout my_brahch
• git fetch --all
• git rebase origin/master
• git push -f origin my_branch (branch name is essential, because otherwise it will force push all branches, which is bad)
  Be careful, force push is a dangerous feature, it should be used only for your private branches, but not for ones which are shared among users (e.g., master)

Implementation-related question will be answered by @NataliaUkhorskaya

[NataliaUkhorskaya]

Yes, as long as it will become a one int field for all flags, you can move isPure to base class. As for new flag: it should be set to true for expressions that uses properties. For example if there this a property val a = 1:

• a - compileTimeConstant for this expression should have this flag to true
• a + 1 - also true

So you should check that the expression isn't a property call and inner expressions don't contain calls to properties

[wutalman]

hey, I pushed the updated version
I added the flag and turned the flags into an int field in CompileTimeConstant, I also changed the inheriting classes' constructors to work with that, and I added the flags to the calls where necessary (mostly in ConstantEvaluator, but in a few other places as well)
Seem to work quite well on what i've tried it on

so now my intention will simplify
2 > 1 -> true
but not
val x = 2
x > 1

[wutalman]

Hey, I pushed some changes to the compile time stuff (not a very meaningful commit message since ill soon squash it with the other and seperate them into the intention and the compile time stuff)

I changed the flag to default to false and instantiate to true from SimpleExpressions

I'm not completely sure how to handle AnnotationValue and ArrayValue, (both in visitCallExpression) do I evaluate a flag for them somehow or do I always instantiate them with false ?

"a" == "a" now works fine, but when i try to evaluate MyEnum.A == MyEnum.A I actually get null (ie nothing to do with the new flag, the evaluator ecaluates just returns with null, i made sure MyEnum.A is evaluated fine (not null and correct flag) but then the comparsion returns null, is that bad ? should i change the logic in the implementation ?

I tried adding tests for the new flag in AbstractEvaluateExpressionTest, but I couldn't use the doTest from isPure since it relies upon using vals which means that it will always evaluate to true.
I wrote a short test of my own instead but it's not working great (because if the bad expectd type i think) is there a more suitable doTest already in use somewhere ?

thanks

[NataliaUkhorskaya]

It will be good if the flag for AnnotationValue and ArrayValue will be true when at least one argument has this flag to true.

What about enums: now I see that MyEnum.A == MyEnum.A isn't a constant expression right now, so evaluator return null for this expression. So it's an another issue, and I'll do it myself, thank you.

What about a test: it checks compileTimeConstant which it written for property initializer, so if in initializer for this property there isn't an another property, the flag should be false. If you always get true it will mean that something works wrong. I think that the problem is that you didn't change a default flag to false in EvaluatorContext class, so the compileTimeConstant for variable has always this flag to true.

[wutalman]

Hi, pushed the latest stuff:

• renamed the tests in more meaningful ways
• added tests for the new flag, seems to be working fine i think

as for ArrayValue: I changed it to check if any of the values assigned to it have the flag set to true and if so set it to true (I didn't change it in the constructor because I was worried it will make in inflexible, i just made it so in all usages (3 instances)), I'm also unsure how to test it, in the test if i do something like prop1 = array(1, 2, 3) it evaluates to null and never gets into the place that creates the ArrayValue, I'm not sure in what case the ArrayValue is being created?

as for AnnotationValue and JavaClassValue: i could not understand how to instantiate those flags, currently they are being set to false by default

as for the intention itself, i changed the assert you pointed at and added the elvis you suggested, but the suggestion to remove the handling for binary expressions and parenthesized expressions doesn't seem to be applicable when you think about it, the intention does logic that the constant evaluation doesn't.
example:
(a && b && true) || (b && true)
if we just check if it can be evaluated to constant the answer is no, we need handling for binary to get inside, now we have
left = (a && b && true)
right = (b && true)
now again for left the constant evaluation will still not evaluate it to a constant and we will need to handle the parentheses, we will simplify what's inside and need to handle those binaries as well etc)
when we're done and we've removed the true we will need to make a decision about whether to keep the parentheses or not (ie (a && b && true) -> (a && b) but (a && true) -> a)

thanks

[NataliaUkhorskaya]

Good work! There are two small notes (see my line comments for the commit).
But also it seems to me that you are going to separate your commit on two - for evaluator and for intention it self, aren't you?

[wutalman]

fixed the inline comments, separated commits

[NataliaUkhorskaya]

Unfortunatly, there are two tests failed. Please, fix them and run 'All tests' configuration before submitting a final version of pull-request.

[wutalman]

Oh sorry, I ran all tests a while ago and I thought the change in compile time wouldn't change it, it was a while ago though, right now i'm getting more than 2 failed tests (I think for reasons not related to this intention) so I'm working on figuring that out

[wutalman]

Hey, I'm looking at the tests that fail and I can't really figure them out, here is an example of one:
in the QuickFixTestsGenerated a test for typeAddition fails (beforePublicFunWithoutBody.kt)

and this is what it says https://gist.github.com/wutalman/02fe57affae75fb3e931, but when i look at the file this is what it contains https://gist.github.com/wutalman/86ea85c358a0a97a0614 and when i check this in the debug instance of IDEA i get http://i.imgur.com/WpuAdYV.png

so basically all the actions that i get in the editor are those that are mentioned in the test, yet it tells me that it gets unexpected ones

do you know what could be causing this ?
thanks

[NataliaUkhorskaya]

We have some problems with QuickFixTestsGenerated on local machines, so you can ignore errors about "Some unexpected actions available.." for now. Have you fixed ControlFlowTestGenerated$ControlStructures and JetDiagnosticsTestGenerated$Tests$Annotations tests?

[wutalman]

fixed the tests you pointed out, rebased it into the 2nd commit (i hope that's okay)

[NataliaUkhorskaya]

You should add fixes of tests to the first commit, because they failed after your changes in it. Also be careful with imports in generated files: now you have changes in JetDiagnosticTestGenerated and if you run 'Generate Tests' run configuration, this file will be modified.

[wutalman]

hey, yes i see what you mean about generate tests now modifying JetDiagnosticTestGenerated, but i'm not sure how to avoid it ?
thanks

[NataliaUkhorskaya]

It seems that you have 'Optimize imports on the fly' option turned on. You can disable it in Settings -> Editor -> Auto Import

[wutalman]

after disabling it, what imports do i remove to make it not change the generated test though ?

[NataliaUkhorskaya]

You can simple rerun 'Generate Tests' run configuration. with this option enabled IDE optimize imports in JetDiagnosticTestGenerated file when you open it and remove unused imports.

[wutalman]

It still creates the JetDiagnosticTestGenerated file when i run

maybe i'm not understanding this correctly, i want it to stop generating a new JetDiagnosticTestGenerated when generate tests is run right? or do i want to commit this new one ?
i tried it both with and without the Optimize imports on the fly option

[NataliaUkhorskaya]

You should run Generate tests and commit the result. If after that you compare JetDiagnosticTestGenerated file with master, there should be changes about new test method and no changes in imports.

[wutalman]

seems the new test is adding two imports that weren't there before
import junit.framework.Assert;
import org.jetbrains.jet.checkers.AbstractJetDiagnosticsTest;

but how can i remove them ? (obviously just removing them won't change anything because they will be generated like that again)

[NataliaUkhorskaya]

As I see in master there are those imports (https://github.com/JetBrains/kotlin/blob/master/compiler/tests/org/jetbrains/jet/checkers/JetDiagnosticsTestGenerated.java), so I don't understand where the problem is.

[wutalman]

okay so it seems the new generated diagnostic test has the same imports as master, i added it to the first commit as well as the fixes for the failing tests, and the second commit is again just the intention

[NataliaUkhorskaya]

Thank you!