Skip to content

Java/C#: Add overrides to the interpretation of neutral MaD models. #17604

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
merged 4 commits into from
Oct 1, 2024

Conversation

aschackmull
Copy link
Contributor

Neutrals generally have little impact on the analysis, but having a neutral like Map.containsKey also cover HashMap.containsKey makes a lot of sense. This might remove a tiny bit of source dispatch for low-confidence dispatch to source implementations of such methods, as that's AFAIR the only touchpoint between neutrals and analysis semantics.

@aschackmull aschackmull requested a review from a team as a code owner September 27, 2024 13:04
@github-actions github-actions bot added the Java label Sep 27, 2024
@michaelnebel
Copy link
Contributor

Would you also be up to making the same change for C# as a part of this PR to align the semantics of C# and Java? (if you prefer I can make the corresponding PR for C# instead).
It appears that some tests are failing.

@aschackmull aschackmull force-pushed the java/neutral-overrides branch from ddffa3f to 689bc21 Compare September 30, 2024 11:21
@aschackmull aschackmull force-pushed the java/neutral-overrides branch from 689bc21 to 0459d13 Compare September 30, 2024 13:17
@aschackmull aschackmull requested a review from a team as a code owner September 30, 2024 13:23
@github-actions github-actions bot added the C# label Sep 30, 2024
@aschackmull
Copy link
Contributor Author

Would you also be up to making the same change for C# as a part of this PR to align the semantics of C# and Java? (if you prefer I can make the corresponding PR for C# instead).

Done. And I've fixed the Java tests. What about the failing C# test? Accept the test changes? I haven't tried to dig into exactly what that test does.

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Sep 30, 2024
@michaelnebel
Copy link
Contributor

Would you also be up to making the same change for C# as a part of this PR to align the semantics of C# and Java? (if you prefer I can make the corresponding PR for C# instead).

Done. And I've fixed the Java tests. What about the failing C# test? Accept the test changes? I haven't tried to dig into exactly what that test does.

Yes, you can accept the updated test output: In this case the updated rows are callables targeted by the neutral MaD rows.

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

Looks good to me - but maybe also run DCA.

@aschackmull
Copy link
Contributor Author

but maybe also run DCA

That seems a bit like potential overkill - neutrals really shouldn't have much effect on analysis, but I guess there is a small chance of impact. I'll kick off a pair of runs.

@aschackmull aschackmull changed the title Java: Add overrides to the interpretation of neutral MaD models. Java/C#: Add overrides to the interpretation of neutral MaD models. Oct 1, 2024
@aschackmull
Copy link
Contributor Author

Dca was fairly uneventful - mostly just telemetry changes, which makes sense.

@aschackmull aschackmull merged commit 6081ba5 into github:main Oct 1, 2024
25 checks passed
@aschackmull aschackmull deleted the java/neutral-overrides branch October 1, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# 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.

2 participants