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: Improve finding best type for models and lifting. #16374

Merged
merged 5 commits into from May 14, 2024

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented May 1, 2024

In this PR

  • The logic for lifting models is improved (further explanation can be seen below).
  • Fix a small issue with sink model exclusions (these were wrongly excluded based on summarized callables and neutrals with summary kind).
  • Do some minor re-factoring.

The idea with the lifting logic is to make models as general as possible.
Assume that B <: A (B is a subtype of A or B is an implementation of A) and assume that we have
methods B.M and A.M where B.M is an override or implementation of A.M. In case the model generator finds a summary model S for B.M we lift the model to A.M (that is, we assume that A.M exhibits the same flow as B.M as this is perceived as a part of the override/implementation contract).

The old logic

  • For lifting was dispersed throughout the code and it was slightly unsound (and unclear).
  • Was not functional. The new implementation replaces superImpl with liftedImpl, which gives us a unique best callable to lift a model to (maybe the callable itself), if it is relevant to make a model for the callable.
  • Lifted models to potential private/internal callables (this was identified by @owen-mc as a part of generating the java SDK models). Now models are only lifted to effectively public API endpoints.

Furthermore, we now only lift models to callables that are also in source. It seems a bit of stretch that if one implements an interface from an imported package and flow is identified for a method then we lift to the callable in the imported package (we could risk getting very odd models for stuff in the Java SDK).

As a test the models for the Java SDK were re-generated and it appears that

  • Models for internal were not generated.
  • Models for methods in eg. AbstractStringBuilder (which is package internal) will be only lifted to the relevant public endpoints.

@github-actions github-actions bot added the Java label May 1, 2024
@michaelnebel michaelnebel force-pushed the java/narrowsuperimpl branch 4 times, most recently from 17b8a0a to 97758eb Compare May 8, 2024 07:38
@github-actions github-actions bot added the C# label May 8, 2024
@michaelnebel michaelnebel marked this pull request as ready for review May 8, 2024 14:33
@michaelnebel michaelnebel requested review from a team as code owners May 8, 2024 14:33
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label May 13, 2024
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.

LGTM

@michaelnebel michaelnebel merged commit 79c6834 into github:main May 14, 2024
34 of 35 checks passed
@michaelnebel michaelnebel deleted the java/narrowsuperimpl branch May 14, 2024 11:12
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.

None yet

2 participants