Skip to content

Java: Fix WildcardTypeAccess.hasNoBound() not considering ArrayTypeAccess #5407

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.

Sign up for GitHub

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

Jump to bottom
Open
Marcono1234 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Java: Fix WildcardTypeAccess.hasNoBound() not considering ArrayTypeAccess #5407

Marcono1234 wants to merge 2 commits into from

Conversation

@Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Mar 13, 2021

@Marcono1234 Marcono1234 requested a review from a team as a code owner March 13, 2021 23:40
@github-actions github-actions bot added the Java label Mar 13, 2021
aschackmull
aschackmull reviewed Mar 16, 2021
View reviewed changes
java/ql/test/library-tests/locations/WildcardLocations.ql
else
// Also makes sure that getBound() is working correctly (and has at most
// one result)
exists(Expr bound | bound = wta.getBound() and not wta.getBound() != bound |
Copy link
Contributor

@aschackmull aschackmull Mar 16, 2021

Choose a reason for hiding this comment

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

Suggested change
exists(Expr bound | bound = wta.getBound() and not wta.getBound() != bound |
exists(Expr bound | bound = wta.getBound() |

It's better to test the not-having-multiple-values by potentially observing those values rather than observing nothing.

Copy link
Contributor Author

@Marcono1234 Marcono1234 Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Though then the test would also succeed when getBound() would have additionally completely unrelated results since here in the test its result is bound below with getUpperBound() / getLowerBound().

aschackmull
aschackmull reviewed Mar 16, 2021
View reviewed changes
aschackmull
aschackmull reviewed Mar 16, 2021
View reviewed changes
java/ql/src/semmle/code/java/Expr.qll Outdated
Expr getLowerBound() { result.isNthChildOf(this, 1) }

/**
* Gets the bound of this wildcard type access, if any, i.e. either the upper
Copy link
Contributor

@aschackmull aschackmull Mar 16, 2021

Choose a reason for hiding this comment

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

Don't use abbreviations in qldoc.

Suggested change
* Gets the bound of this wildcard type access, if any, i.e. either the upper
* Gets the bound of this wildcard type access, if any, that is, either the upper

Copy link
Contributor Author

@Marcono1234 Marcono1234 Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Made this a separate sentence ("... any. That is, ...") to be consistent with other comments and to not have so many commas.
(Though this might not be grammatically correct?)

aschackmull
aschackmull reviewed Mar 16, 2021
View reviewed changes
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

Some inline comments, otherwise LGTM.

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Mar 16, 2021
@aschackmull
Copy link
Contributor

aschackmull commented Mar 16, 2021 

Some tests failed!
    ql/java/ql/test/library-tests/typeaccesses/ArrayTypeAccesses.ql: Extractor error (Extraction failed)
    ql/java/ql/test/library-tests/typeaccesses/PrintAst.qlref: Extractor error (Extraction failed)
    ql/java/ql/test/library-tests/typeaccesses/TypeAccesses.ql: Extractor error (Extraction failed)

@Marcono1234
Copy link
Contributor Author

Marcono1234 commented Mar 22, 2021 •
edited
Loading

Thanks for the feedback and sorry for the oversights. I assume the test failures came from a compilation error in the source test file; I overlooked this and CodeCLI was not failing locally for me (see #5476).

I have addressed most of the feedback except the wta.getBound() check (see comment).

If you decide to merge this pull request, then please, in your own interest, squash the commits for a clean history on main.
If you want I can in the future also use fixup commits, though I don't think GitHub automatically squashes them when merging branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aschackmull aschackmull aschackmull left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Java no-change-note-required This PR does not need a change note

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Java] WildcardTypeAccess.hasNoBound() returns wrong result for array bounds

2 participants

@Marcono1234 @aschackmull