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

Add whole-program inference mode using stub files instead of jaif files #2871

Merged
merged 400 commits into from
May 23, 2020

Conversation

kelloggm
Copy link
Contributor

@kelloggm kelloggm commented Nov 8, 2019

This PR adds a command-line options to support using .astub files as the output of whole-program inference instead of .jaif files, and permits -Ainfer to take an argument:

  • -AmergeStubsWithSource causes stub types to be used, even when source files are available; the stronger type from the two is used.

For now, there is scant documentation on -Ainfer=stubs, because there is no practical way for a user to make use of it (for now). This is because the manual only describes how to run whole-program inference via the infer-and-annotate.sh script, which is not practical for large programs. Providing a version of infer-and-annotate.sh that uses stubs (or something more scalable) is explicitly future work for this PR.

The stub mode of WPI is tested with the same tests that the .jaif version of whole-program inference uses. Those tests also implicitly test -AmergeStubsWithSource, because it's not possible to overwrite (a lack of) annotations in source files otherwise.

Merge together with typetools/annotation-tools#279

@kelloggm kelloggm requested a review from smillst November 8, 2019 00:54
@smillst smillst assigned kelloggm and smillst and unassigned smillst and kelloggm Nov 8, 2019
@smillst
Copy link
Member

smillst commented Nov 8, 2019

The commit history is a bit funky because I merged an earlier version of #2846 and built off of that. Those commits claim they've been merged, but they're still showing up in the diff here. I've tried getting rid of them, and they're required for my tests to pass locally. @smillst, were those merged? If not, can I merged them here? Or do they need separate tests?

I think I fixed this. Can you double check the diffs?

@kelloggm
Copy link
Contributor Author

kelloggm commented Nov 8, 2019

Yes, that works now. Thanks, I appreciate it.

It looks like some of the all-systems tests are failing; I'll look into that and then re-assign you when tests are passing for review.

@smillst smillst assigned kelloggm and unassigned smillst Nov 8, 2019
@kelloggm kelloggm assigned smillst and unassigned kelloggm Nov 13, 2019
Copy link
Member

@smillst smillst left a comment

Choose a reason for hiding this comment

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

I haven't looked at all the code yet. Could you please add a comment for every method, even private ones?

import scenelib.annotations.util.Strings;

/**
* SceneToStubWriter provides two static methods named <code>write</code> that write a given {@link
Copy link
Member

Choose a reason for hiding this comment

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

Use {@code } instead of the html tag.

// do so is here.
wholeProgramInference.saveResults();
WholeProgramInference.OutputKind outputKind =
checker.hasOption("outputStubs")
Copy link
Member

Choose a reason for hiding this comment

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

I would change -AoutputStubs to -Ainfer=stubs. Then this would be checker.getOption("infer").equals("stubs"). That makes documenting this option easier.

AnnotatedTypeMirror stubType = stubTypes.getAnnotatedTypeMirror(elt);
// This is a bit of a hack - need to decide if the annotated type mirror
// contains ANY annotations - i.e. on deep types, method params, etc.
boolean stubTypeHasAnAnnotation = stubType != null && stubType.toString().contains("@");
Copy link
Member

Choose a reason for hiding this comment

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

Replace with a TypeScanner:

        new SimpleAnnotatedTypeScanner<Boolean, Void>() {
            @Override
            protected Boolean defaultAction(AnnotatedTypeMirror type, Void aVoid) {
                return type.getAnnotations().isEmpty();
            }

            @Override
            protected Boolean reduce(Boolean r1, Boolean r2) {
                r1 = r1 == null ? false : r1;
                r2 = r2 == null ? false : r2;
                return r1 || r2;
            }
        };

docs/manual/introduction.tex Outdated Show resolved Hide resolved
@smillst smillst assigned kelloggm and unassigned smillst Nov 13, 2019
@kelloggm
Copy link
Contributor Author

I've added javadoc to all the methods and fields, even the private ones, and made the changes @smillst suggested (with one exception: I was able to avoid the need to scan for the presence of annotations in general, so I could remove the ugly string hack entirely without replacing it with a SimpleAnnotatedTypeScanner). @smillst can you take another look?

@kelloggm kelloggm assigned smillst and unassigned kelloggm Nov 18, 2019
@smillst smillst assigned kelloggm and unassigned smillst Nov 19, 2019
@smillst
Copy link
Member

smillst commented Nov 19, 2019

@kelloggm Please update the pull request description to match the implementation.

@kelloggm kelloggm assigned smillst and unassigned kelloggm Nov 19, 2019
@smillst smillst assigned kelloggm and unassigned smillst Nov 19, 2019
@kelloggm kelloggm removed their assignment Nov 19, 2019
mernst and others added 25 commits May 15, 2020 15:08
…move-ascenewrapper-remove-vivifyAnd into scene-to-stub-writer-remove-ascenewrapper-remove-vivifyAnd-2
…into scene-to-stub-writer-remove-aclasswrapper
…move-aclasswrapper into scene-to-stub-writer-remove-ascenewrapper
…move-ascenewrapper into scene-to-stub-writer-remove-ascenewrapper-remove-vivifyAnd
…move-ascenewrapper-remove-vivifyAnd into scene-to-stub-writer-remove-ascenewrapper-remove-vivifyAnd-2
…move-ascenewrapper into scene-to-stub-writer-remove-ascenewrapper-remove-vivifyAnd
…move-ascenewrapper-remove-vivifyAnd into scene-to-stub-writer-remove-ascenewrapper-remove-vivifyAnd-2
…into scene-to-stub-writer-remove-ascenewrapper
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.

None yet

4 participants