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

Support emitting "sourceType" diagnostics #36

Merged
merged 4 commits into from
Jul 26, 2023

Conversation

netdpb
Copy link
Collaborator

@netdpb netdpb commented Jul 19, 2023

Add a new flag: -AshowTypes. When set, the type of each expression whose value is used in an assignment-like context is emitted as a warning, for use by conformance tests.

In a future commit, the flag will also cause the expected type of the receiver to be emitted as well.

@netdpb netdpb requested review from cpovirk and wmdietl July 19, 2023 21:28
@netdpb
Copy link
Collaborator Author

netdpb commented Jul 19, 2023

@wmdietl Does this new flag seem reasonable? Should it have a better name? Is this the kind of thing that EISOP would be able to support?

@wmdietl
Copy link
Collaborator

wmdietl commented Jul 20, 2023

@wmdietl Does this new flag seem reasonable? Should it have a better name? Is this the kind of thing that EISOP would be able to support?

@netdpb In EISOP we have the flag -AlspTypeInfo to output type information for LSP hover notifications. It might actually be directly usable for this purpose as well or at least it should be relatively easy to generalize for this usage.
It's implemented here:

Using that here is blocked by #29, but hopefully I'll wrap that up soon.

@netdpb
Copy link
Collaborator Author

netdpb commented Jul 21, 2023

I don't want to make extra work, but I also don't want to block the ability to write these new assertions. I'd like to continue with this approach, and then work to adapt the logic when we start depending on EISOP. Does that make sense?

Separately, it looks like TypeInformationPresenter reports the expected "sink" type for assignments, but not for method call assignment slots. That's the next thing I'd want to be able to report. I'd imagine that's something we could add, with a new MessageKind. Right?

@wmdietl
Copy link
Collaborator

wmdietl commented Jul 24, 2023

I don't want to make extra work, but I also don't want to block the ability to write these new assertions. I'd like to continue with this approach, and then work to adapt the logic when we start depending on EISOP. Does that make sense?

Sure.

Separately, it looks like TypeInformationPresenter reports the expected "sink" type for assignments, but not for method call assignment slots. That's the next thing I'd want to be able to report. I'd imagine that's something we could add, with a new MessageKind. Right?

You will get two messages for method calls, one with the declared type of the method and one with the use type of the method.
Why two types? Generics. Take

class GenericCall {
    <T> T id(T p) { return p; }

    void foo() {
        String s = id("hi");
    }
}

You'll get two messages for the call of id:

GenericCall.java:5: error: [lsp.type.information] checker=NullnessChecker; kind=DECLARED_TYPE; type=<T/*DECL*/ [ extends @Initialized @Nullable Object super @Initialized @NonNull Void]> T[ extends @Initialized @Nullable Object super @Initialized @NonNull Void] id(@Initialized @NonNull GenericCall this, T[ extends @Initialized @Nullable Object super @Initialized @NonNull Void] p0); range=(4, 19, 4, 20)
        String s = id("hi");
                   ^
GenericCall.java:5: error: [lsp.type.information] checker=NullnessChecker; kind=USE_TYPE; type=<T/*DECL*/ [ extends @Initialized @Nullable Object super @Initialized @NonNull Void]> @Initialized @NonNull String id(@Initialized @NonNull GenericCall this, @Initialized @NonNull String p0); range=(4, 19, 4, 20)
        String s = id("hi");
                     ^

You would probably want the use type of the invoked method, as that is what needs to be compared against the call argument.
So the information you need is there, just not broken down by the individual parameter - argument pairing.
For the LSP functionality this seems okay, but feel free to file an issue to split this up properly.

@netdpb
Copy link
Collaborator Author

netdpb commented Jul 26, 2023

ou would probably want the use type of the invoked method, as that is what needs to be compared against the call argument.
So the information you need is there, just not broken down by the individual parameter - argument pairing.
For the LSP functionality this seems okay, but feel free to file an issue to split this up properly.

Aha! This makes sense. I think that as long as the information is parsable so that it's possible to extract each parameter's information, that would be enough.

Copy link
Collaborator

@wmdietl wmdietl 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!

@netdpb netdpb merged commit 763b130 into jspecify:main Jul 26, 2023
@netdpb netdpb deleted the expression-type branch July 26, 2023 20:04
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.

3 participants