Skip to content
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

Java: Fix some predicates being incorrect for Records #5473

Closed

Conversation

@Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Mar 21, 2021

Properly implements the predicates for Record classes and adds tests.

Note: At the current state this pull request is pointless because Modifiable.hasModifier reports implicit modifiers (see #5472); once this has been fixed, this pulll request is necessary.

@Marcono1234 Marcono1234 requested a review from as a code owner Mar 21, 2021
@github-actions github-actions bot added the Java label Mar 21, 2021
@aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Mar 22, 2021

File "ql/java/ql/src/semmle/code/java/Type.qll" contains a non-ASCII character at the location marked with | in:

 extends Class {
  Record() { isRecord(this) }

  // JLS 16 |

ASCII check failed!

@Marcono1234
Copy link
Contributor Author

@Marcono1234 Marcono1234 commented Mar 22, 2021

Yeah, I know, I added the the section sign § on purpose...
I wondered why the other QLDoc comment wrote out "section" in full. Sorry for this trouble, but I really could not have known that there is an ASCII-only restriction.
I don't think there is any document here mentioning it (or I missed it), the QL spec apparently has no such restrictions and locally for me the query library is compiled just fine. (another reason for #5480)

If you want I can replace / remove those §, though would it make sense to relax this restriction to allow ISO-8859-1 / Latin 1 (which § is part of), maybe excluding undefined chars and the NBSP?

Note however, that as mentioned in the description there is currently little value in merging this pull request.

Sorry that this comment is somewhat reproachful, it just feels so uncomfortable to create pull requests which fail, wasting your and my time. If I would at least see the CI failure I could (hopefully) fix problems because you even have a first look on it.
(Though some of my previous pull requests were really in a bad state which I could have detected myself locally.)

@smowton
Copy link
Contributor

@smowton smowton commented Sep 3, 2021

Tested this against the latest extractor: can confirm the tests pass without the QL changes (i.e., the extractor is populating the implicit modifiers for us)

@smowton
Copy link
Contributor

@smowton smowton commented Sep 6, 2021

Just the test merged as #6604, as the extractor is already flagging these classes as static and/or final for us.

@smowton smowton closed this Sep 6, 2021
@Marcono1234
Copy link
Contributor Author

@Marcono1234 Marcono1234 commented Sep 12, 2021

@smowton, thanks for merging the tests. Ideally the NestedType.isStatic() documentation would be updated like it was proposed by this pull request. However, the question is if there is any point in overriding the predicate in the first place when implicit modifiers are reported anyways. As mentioned in #5682 (comment) there is probably some cleanup need to remove this special casing. Do you want me to create a new issue for that?

@smowton
Copy link
Contributor

@smowton smowton commented Sep 13, 2021

Happy to merge a docs change (don't use the section character §, since the codeql source code is intentionally entirely ASCII). Probably worth noting that the extractor provides some of the advertised functionality.

On cleaning up any duplication between the extractor and the QL library, I'm disinclined to spend time on it: we can only possibly make the situation worse for users, and I'll need to check I've written an exhaustive suite of tests to be sure I haven't introduced a regression.

@Marcono1234
Copy link
Contributor Author

@Marcono1234 Marcono1234 commented Oct 2, 2021

@smowton, I have instead created #6797 now to remove the overwritten QLDoc because it adds in my opinion not much value (and in other cases it is not listed either which implicit modifiers are detected).

@Marcono1234 Marcono1234 deleted the marcono1234/record-predicates branch Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants