Skip to content

[Java] Add CharacterLiteral to CompileTimeConstantExpr.getStringValue #8325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

JLLeitschuh
Copy link
Contributor

Simplifies end-user logic when looking for charater literals.

@JLLeitschuh JLLeitschuh requested a review from a team as a code owner March 3, 2022 22:59
@github-actions github-actions bot added the Java label Mar 3, 2022
Comment on lines 18 to 24
int paren = (12);
String string = "a string";
int ternary = (3 < 5) ? 1 : 2;
char charLiteral = 'a';

return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this file tab delimited when the rest are space delimited? 🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to fix this as a part of this PR 😆 someone might want to make a sweep though, just my 10c

@JLLeitschuh JLLeitschuh force-pushed the feat/JLL/improve_compile_time_constant branch from 4f23a93 to 04cd0db Compare March 3, 2022 23:08
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JLLeitschuh, thanks for your contribution. This mostly LGTM, I added a couple of inline comments.

JLLeitschuh and others added 2 commits March 4, 2022 09:29
Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com>
@JLLeitschuh JLLeitschuh requested a review from atorralba March 4, 2022 20:17
@JLLeitschuh
Copy link
Contributor Author

This PR is done and can be merged whenever you're comfortable doing so. 😄

@atorralba atorralba merged commit 08ce128 into github:main Mar 7, 2022
@Marcono1234
Copy link
Contributor

This change breaks string concatenation logic for CompileTimeConstantExpr and might also negatively affect custom user queries. Consider for example 'a' + 'b'; with this change getStringValue() erroneously has "ab" as result, while the correct result is an int representing ((int) 'a') + ((int) 'b') (unfortunately this does not seem to be covered by any tests).
Currently the logic in CompileTimeConstantExpr is also incomplete, but once it gets closer to the JLS, this would affect more cases, such as 'a' + 1.
Similarly a user who is using getStringValue() in their query will likely expect that the expression is really of type String, and not of type char / Character, which is incompatible with String.

It looks like the reason why this change to getStringValue() was proposed, was to simplify a very specific case where a CodeQL expression represents a read of a field of type char or String, see https://github.com/github/codeql/pull/8032/files#r818222618.
Would it therefore make sense to revert the changes done to getStringValue(), and instead modify the code for that specific use case? To me that looks like a saner solution.

@smowton
Copy link
Contributor

smowton commented Mar 7, 2022

Hm, sure, the concat case clearly needs fixing. Concat of string + char is nice to have though for example. @JLLeitschuh how about amending to have a separate getStringifiedValue, and use that to handle concatenation in the particular case where the resulting expression is of type String? Thus 'a' wouldn't have a getStringValue (but would have a getStringifiedValue), but "a" + 'b' would have both.

getStringifiedValue is hard to implement in general of course since e.g. floating point numbers are likely to stringify differently to how the user wrote them in source (e.g. final double d = 1.0e1 will stringify as 10.0), but it would be acceptable in my view to give a partial implementation so long as that is clearly noted in getStringifiedValue's qldoc.

@Marcono1234
Copy link
Contributor

getStringifiedValue is hard to implement in general of course since e.g. floating point numbers are likely to stringify differently to how the user wrote them in source (e.g. final double d = 1.0e1 will stringify as 10.0), but it would be acceptable in my view to give a partial implementation so long as that is clearly noted in getStringifiedValue's qldoc

To me that looks acceptable as well (if the qldoc describes that the value is converted to string), because in Java for example "a" + 1.0e1 is "a10.0", so this would not be that different from what one might expect.

JLLeitschuh added a commit to JLLeitschuh/ql that referenced this pull request Mar 8, 2022
Removes CharacterLiteral from CompileTimeConstantExpr.getStringValue

Resolves:
 - github#8325 (comment)
 - github#8325 (comment)
JLLeitschuh added a commit to JLLeitschuh/ql that referenced this pull request Mar 8, 2022
Removes CharacterLiteral from CompileTimeConstantExpr.getStringValue

Resolves:
 - github#8325 (comment)
 - github#8325 (comment)
JLLeitschuh added a commit to JLLeitschuh/ql that referenced this pull request Mar 8, 2022
Removes CharacterLiteral from CompileTimeConstantExpr.getStringValue

Resolves:
 - github#8325 (comment)
 - github#8325 (comment)
JLLeitschuh added a commit to JLLeitschuh/ql that referenced this pull request Mar 8, 2022
Removes CharacterLiteral from CompileTimeConstantExpr.getStringValue

Resolves:
 - github#8325 (comment)
 - github#8325 (comment)
JLLeitschuh added a commit to JLLeitschuh/ql that referenced this pull request Mar 8, 2022
Removes CharacterLiteral from CompileTimeConstantExpr.getStringValue

Resolves:
 - github#8325 (comment)
 - github#8325 (comment)
@JLLeitschuh
Copy link
Contributor Author

Thanks for the feedback! Resolved here: #8360

atorralba added a commit that referenced this pull request Mar 11, 2022
Java: Revert #8325, Add CharacterLiteral to CompileTimeConstantExpr.getStringValue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants