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

Sort upstream facts based on package paths #68

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

yuxincs
Copy link
Contributor

@yuxincs yuxincs commented Oct 5, 2023

We are calling analysis.Pass.AllPackageFacts() API from the analysis framework to import NilAway facts from upstream dependencies. However, this API is documented as

AllPackageFacts returns a new slice containing all package facts of the analysis's FactTypes in unspecified order.

In some drivers such as nogo, this order is deterministic, however, in most other drives like the one from the analysis frameowork, the order will be nondeterministic.

Therefore, for determinism in NilAway, this PR sorts imported upstream facts (i.e., inference.InferredMap) based on the package path.

Depends on PR #67

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #68 (3a3d09e) into main (ce694d0) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
+ Coverage   89.23%   89.27%   +0.03%     
==========================================
  Files          54       54              
  Lines        8233     8239       +6     
==========================================
+ Hits         7347     7355       +8     
+ Misses        728      727       -1     
+ Partials      158      157       -1     
Files Coverage Δ
inference/engine.go 96.44% <100.00%> (+0.66%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@sonalmahajan15 sonalmahajan15 left a comment

Choose a reason for hiding this comment

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

LGTM, modulo the question asked below.

go.mod Outdated Show resolved Hide resolved
@yuxincs yuxincs force-pushed the yuxincs/use-ordered-map-for-inferred-map branch from d745880 to 658582b Compare October 10, 2023 17:47
@yuxincs yuxincs force-pushed the yuxincs/sort-upstream-facts branch from ec3931c to f06151d Compare October 10, 2023 17:47
@yuxincs yuxincs force-pushed the yuxincs/use-ordered-map-for-inferred-map branch from 658582b to c0f0e58 Compare October 11, 2023 14:25
@yuxincs yuxincs force-pushed the yuxincs/sort-upstream-facts branch from f06151d to 314845f Compare October 11, 2023 14:25
Copy link
Contributor

@sonalmahajan15 sonalmahajan15 left a comment

Choose a reason for hiding this comment

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

LGTM!

@yuxincs yuxincs force-pushed the yuxincs/use-ordered-map-for-inferred-map branch from c0f0e58 to cec86d2 Compare October 17, 2023 20:46
@yuxincs yuxincs force-pushed the yuxincs/sort-upstream-facts branch from 314845f to 36832a1 Compare October 17, 2023 20:47
@yuxincs yuxincs force-pushed the yuxincs/use-ordered-map-for-inferred-map branch from cec86d2 to 830bd2b Compare October 18, 2023 16:50
@yuxincs yuxincs force-pushed the yuxincs/sort-upstream-facts branch from 36832a1 to 58fe1b5 Compare October 18, 2023 16:50
@yuxincs yuxincs force-pushed the yuxincs/use-ordered-map-for-inferred-map branch from 830bd2b to dc0e5a7 Compare October 29, 2023 16:07
@yuxincs yuxincs force-pushed the yuxincs/sort-upstream-facts branch from 58fe1b5 to 3ebe927 Compare October 29, 2023 16:07
@yuxincs yuxincs force-pushed the yuxincs/use-ordered-map-for-inferred-map branch from dc0e5a7 to 3782547 Compare October 31, 2023 15:11
Base automatically changed from yuxincs/use-ordered-map-for-inferred-map to main October 31, 2023 15:21
@yuxincs yuxincs force-pushed the yuxincs/sort-upstream-facts branch from 3ebe927 to 73181c5 Compare October 31, 2023 15:22
@yuxincs yuxincs merged commit aa0f172 into main Oct 31, 2023
4 checks passed
@yuxincs yuxincs deleted the yuxincs/sort-upstream-facts branch October 31, 2023 15:31
yuxincs added a commit that referenced this pull request Oct 31, 2023
This PR fixes two panics due to the assumption that the RHS of the
global variable assignments are always variables, which isn't true when
RHS is a constant (either defined in the current package, or referenced
from upstream package)

We also take this opportunity to refactor the code a little bit to be
more readable and maintainable.

Depends on #68
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