feature: Support of shift left and right operation inside VisitorPartialEvaluator

[thimo-seitz]

Support for evaluation of shift left and right operations:

• 1L << 53
• 8 >> 2

I've found no matching issue for this.

[monperrus]

Thanks a lot for the contribution. Would you be able to specify this in the appropriate test case?

[slarse]

Hi @thimo-seitz,

Thank you for your contribution, and welcome to Spoon!

If there's an issue related to this could you link it? If there's no issue, could you write a brief summary in the PR body about what you're adding here?

As per the contributing guidelines, new features must be accompanied by test cases. Could you add tests for the new functionality? I think the tests for this stuff are in EvalTest.java, they should be pretty straightforward to write.

Also see my couple of comments on the code.

[MartinWitt]

@monperrus I believe you must approve a first time committer before any workflow runs.

[monperrus]

Thanks a lot for the tests.

LGTM, will merge.

[slarse]

@monperrus Hold that thought, I want to have a look :)

[slarse]

Implementation looks good, but the tests for short and byte don't actually hit the expected code paths.

@monperrus Not really related to this PR, but what do you think about the fact that getType() doesn't care about casts? I.e. the type of the expression (byte) 1 is actually int. Bug or feature?

[slarse]

There is a problem with this test, as well as the one for short types (same deal for right shifting), namely that they actually get int as the lhs in the evaluation. We can see this from the fact that the code paths that should be hit, actually arent: https://coveralls.io/builds/43661711/source?filename=src%2Fmain%2Fjava%2Fspoon%2Fsupport%2Freflect%2Feval%2FVisitorPartialEvaluator.java#L214

The reason for this is arguably due to incorrect semantics in Spoon. When the type of an expression is taken, it's the type of the expression without the type casts. I.e. element.getType() on an element with a cast on it returns the type of the element without the cast, which is a bit strange to me. But it's what happens right now.

We can work around this by manually constructing the binary operator, which arguably is the way all of these operators should be constructed (because right now these tests depend on the correctness of compiling code snippets, which is completely unrelated functionality). That could be done for example with a helper like this:

	private CtBinaryOperator<?> createBinaryOperatorOnLiterals(
			Factory factory, Object leftLiteral, Object rightLiteral, BinaryOperatorKind opKind) {
		return factory.createBinaryOperator(
				factory.createLiteral(leftLiteral), factory.createLiteral(rightLiteral), opKind);
	}

And calling it like so:

Suggested change

	CtCodeElement el = launcher.getFactory().Code().createCodeSnippetExpression("((byte)0b00000010)<<2").compile();
	CtCodeElement el = createBinaryOperatorOnLiterals(
	launcher.getFactory(), (byte) 2, 2, BinaryOperatorKind.SL);

That would allow us to hit the expected code paths for byte and short. On a side note, I'm not sure why you wrote the 2 in binary with a whole bunch of leading digits, but I think it's better if you just write 2 :)

[thimo-seitz]

I did a little research in the Java specification. The branches for byte and short are not really needed, because they are converted to int: https://docs.oracle.com/javase/specs/jls/se11/html/jls-5.html#jls-5.6.1
You would only have to consider long and treat everything else as int.

On a side note, I'm not sure why you wrote the 2 in binary with a whole bunch of leading digits, but I think it's better if you just write 2 :)

Some sort of "eight bits are a byte"-fetish :)

[thimo-seitz]

I implemented the strict creation of the types, but i think it isn't really necessary, since a bitwise shift would always result in int or long as the spec says. Maybe i should remove the paths for byte and short and treat these as int also?

[slarse]

https://docs.oracle.com/javase/specs/jls/se11/html/jls-5.html#jls-5.6.1
You would only have to consider long and treat everything else as int.

Very nice catch, I did not know that.

Maybe i should remove the paths for byte and short and treat these as int also?

Yes, this is the best way to solve it. Only do special handling of long, otherwise assume int.

I implemented the strict creation of the types, but i think it isn't really necessary,

Given what we know now, no it isn't strictly necessary. They are also so different from the other test inputs (which I plan to turn into a parameterized test) that removing them is beneficial for maintainability. Had they not required the workaround, I would have said to keep them for the sake of documentation, but as it stands I think they're too much effort.

[thimo-seitz]

I have modified it so that byte and short still work. I know too little about the system, whether byte or short can occur there as leftObject. Or is that only possible via the constructed test case?

[slarse]

Byte and short can occur on the left-hand side, and the "forcibly constructed" case is also a valid way to build a Spoon model. So, yes :)

[lgtm-com]

This pull request introduces 2 alerts when merging eaa3e89 into f89b693 - view on LGTM.com

new alerts:

• 2 for Dereferenced variable may be null

[thimo-seitz]

This pull request introduces 2 alerts when merging eaa3e89 into f89b693 - view on LGTM.com

new alerts:

* 2 for Dereferenced variable may be null

The isIntegralType-check before catches the null case. What should i do?

[slarse]

LGTM, thanks @thimo-seitz!

The isIntegralType-check before catches the null case. What should i do?

I agree, it's a false positive. We ignore it.

@monperrus Please merge if tests go green, I don't have time to wait for CI and I'm away tomorrow :)

[slarse]

Never mind, time flew by and suddenly CI was done.

[slarse]

Thanks @thimo-seitz, congrats on your first contribution to Spoon!