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 WildcardTypeAccess.hasNoBound() not considering ArrayTypeAccess #5407

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

Conversation

Marcono1234
Copy link
Contributor

@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
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

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

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().

@@ -1742,11 +1742,27 @@ class WildcardTypeAccess extends Expr, @wildcardtypeaccess {
/** Gets the lower bound of this wildcard type access, if any. */
Expr getLowerBound() { result.isNthChildOf(this, 1) }

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

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

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?)

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

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

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
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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