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

Simplify the parameter checks for overrides. #829

Closed
wants to merge 5 commits into from

Conversation

cpovirk
Copy link

@cpovirk cpovirk commented Aug 6, 2024

DO NOT MERGE, probably.

See discussion from about
jspecify/jspecify-reference-checker#193 (comment)
onward.

This PR is a lazy way for me to run tests, since I don't trust myself to
find the right way to run them all locally.

DO NOT MERGE, probably.

See discussion from about
jspecify/jspecify-reference-checker#193 (comment)
onward.

This PR is a lazy way for me to run tests, since I don't trust myself to
find the right way to run them all locally.
@cpovirk cpovirk force-pushed the simplifyoverrideparamcheck branch from f1d50e8 to 85c2470 Compare August 7, 2024 01:53
@cpovirk
Copy link
Author

cpovirk commented Aug 7, 2024

For the record, for cpovirk@85c2470 (typeHierarchy.isSubtype(overriddenParams.get(i), overriderParams.get(i)); and nothing else), I'm seeing:

ERROR: internal error in type processor! method typeProcessOver() doesn't get called.

I might now test a variant of that.

@cpovirk
Copy link
Author

cpovirk commented Aug 7, 2024

OK, I see the same problem if I merely pass overriddenParams.get(i) to isSubtype instead of passing the result of applyCaptureConversion (i.e., if I keep that part of my original change but revert the removal of the testTypevarContainment check).

@cpovirk
Copy link
Author

cpovirk commented Aug 7, 2024

...and the same problem if I put the applyCaptureConversion call back but remove the testTypevarContainment check.

I'm sure it could be interesting to dig deeper, but I'm just going to take away from this that there's no code that became trivially removable somewhere along the way.

@cpovirk cpovirk closed this Aug 7, 2024
@cpovirk
Copy link
Author

cpovirk commented Aug 14, 2024

(#849 suggests that the errors I'm seeing may or may not actually have been caused by the changes I was testing. For my current purposes, that still means that the code is not "trivially removable." But maybe I'll revisit it someday, assuming my investigation in jspecify/jspecify#583 doesn't make the changes look like a non-starter.)

@cpovirk cpovirk reopened this Oct 30, 2024
@cpovirk
Copy link
Author

cpovirk commented Oct 30, 2024

OK, that version (which wasn't my original version) fails one test that seems potentially unrelated:

I18nFormatterTest > run[D:\a\checker-framework\checker-framework\checker\tests\i18n-formatter] FAILED
    java.lang.AssertionError: 69 out of 70 expected diagnostics were found.
    1 unexpected diagnostic was found:
      I18nFormat.java:18: error: (i18nformat.string.invalid)
    1 expected diagnostic was not found:
      I18nFormat.java:18: warning: (i18nformat.missing.arguments)
    While type-checking D:\a\checker-framework\checker-framework\checker\tests\i18n-formatter\ConversionCategoryTest.java, D:\a\checker-framework\checker-framework\checker\tests\i18n-formatter\HasFormat.java, D:\a\checker-framework\checker-framework\checker\tests\i18n-formatter\I18nFormat.java, D:\a\checker-framework\checker-framework\checker\tests\i18n-formatter\I18nFormatForTest.java, D:\a\checker-framework\checker-framework\checker\tests\i18n-formatter\IsFormat.java, D:\a\checker-framework\checker-framework\checker\tests\i18n-formatter\ManualExampleI18nFormatter.java, D:\a\checker-framework\checker-framework\checker\tests\i18n-formatter\Syntax.java

I'm going to see what happens with the original version.

@wmdietl
Copy link
Member

wmdietl commented Oct 30, 2024

OK, that version (which wasn't my original version) fails one test that seems potentially unrelated:

Yes, unfortunately CI currently fails on master, with that same test case, after I merged a green PR.
It seems to only occur on the MacOS/Windows tests, so I'm not sure yet what is going on.

@cpovirk
Copy link
Author

cpovirk commented Oct 30, 2024

Thanks.

It looks like the original version fails only in that similar way, so I think everything I've seen is still at least compatible with the idea that this code could be simplified.

Anyway, I don't want to actively pick this up now; I just remembered seeing that the earlier CI failure had been fixed, so I figured I'd resurrect this PR for testing purposes. Whenever I get back to jspecify/jspecify-reference-checker#193, I may end up wanting to push this forward, or I might not.

@cpovirk cpovirk closed this Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants