You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
If I hover over "object" inside this if block it says Object object, but the type here is known to be Instance (and the set method is declared on Instance).
@bwilkerson Is there value in showing Object to the user here? I could add the staticType in addition but it might be weird if the tooltip shows both Object and Instance. However, I can't (nicely) remove Object because of it being embedded in elementDescription.
It certainly makes sense to tell the user that the static type is "Instance" here, but there might be some value in also telling them how it was declared. Maybe it's largely a question of presentation. I don't know what the current hover text looks like, but perhaps something like this would make sense:
The static type is Instance.
It was declared as Object object.
(I say "like this" because I don't think the above is either attractive or readable.)
What I'd really like us to be able to display is the information to explain the difference in these two types (something like "because of the 'is' check on line lineNumber").
But it's possible that we ought to make the declared type and the element name be two different fields for ease of composition. (We have yet to find the sweet spot between making it flexible by having lots of fields and reducing the work on the client side by pre-formatting the output.)
We have yet to find the sweet spot between making it flexible by having lots of fields and reducing the work on the client side by pre-formatting the output
Yeah, this comes up now and I was reminded of it when I saw this.
The tooltip for this currently looks kinda sparse, because it's a local variable with no docs or anything:
What I'd really like us to be able to display is the information to explain the difference in these two types (something like "because of the 'is' check on line lineNumber").
Yeah, that'd be neat. I'd love to show it as something like this:
I think putting the static type first makes sense since it's probably the one the user cares most about here, but with a note about the declared type and why it's different.
Looking in our hover code, I noticed we have something called the propagatedType:
if (propagatedType) displayString += `propagated type: ${propagatedType.trim()}`;
I'm not sure I remember every seeing that rendered, so not sure what it is (the server docs just say it's the propagated type, but I'm none the wiser). For now, I'm gonna stick the static type on the end in parens:
Object object (Instance)
And I'm moving propagated type to the same (since where it's currently rendered the label is fixed-with and looks wonky if I force it to show). Hopefully both won't appear at the same time as that'll be a little confusing!
Ugh, the static type is provided even when it matches the start of elementDescription which means I end up with String a (String) on a normal string. In order to show the static type (and not look redundant), I'd need to know whether it's different, and I don't think I can do that reliably (I could check it starts with the static type then a space, but I think that might not be reliable since there could be spaces in function types?).
I think this may need a server change before we can really improve it.
... I noticed we have something called the propagatedType ...
As of Dart 2, there shouldn't be any propagated type information (I need to deprecate the field). The propagated type was an early, analyzer-only, attempt to have better type information than the spec allowed for at that time. Now that we have type inference, we no longer compute a propagated type.
Ugh, the static type is provided even when it matches the start of elementDescription ...
I can think of three ways to resolve that problem, two of which I'd already mentioned. First, we could split the elementDescription into a declaredType and name pair. Second, we could provide a single description field that combined all of the information for clients so that they don't need to compose it for themselves. Third, we could omit the staticType field when there's no useful information.
We need to think about cases where there is no declared type (such as var x = ...), cases like the one above where the declared and static type are significantly different (Object vs Instance), and cases where the static type is just a refinement (List vs List<String>). And, of course, we shouldn't forget the cases where the variable isn't declared at all.
As of Dart 2, there shouldn't be any propagated type information (I need to deprecate the field). The propagated type was an early, analyzer-only, attempt to have better type information than the spec allowed for at that time.
Ah, makes sense (and why I haven't seen it lately).
I can think of three ways to resolve that problem, two of which I'd already mentioned.
I don't have any strong preferences on this. Having separate fields is more flexible, but I also like that giving us formatted strings gives a consistency across tools (and also I think people working on the server have a much better understanding of Dart than I, so are likely to format things better than me mashing a bunch of fields together). Any implementation that allows the user to see this extra info - which is a really neat feature of Dart (though it slightly bugs me that it doesn't work if in invert the condition - not sure if that's a bug?) - would be great. I can open an SDK bug if you think it should be tracked over there.
... though it slightly bugs me that it doesn't work if in invert the condition - not sure if that's a bug?
You and a lot of people :-). There was an effort started to enhance the flow of type information through code structures, but it keeps not being at the top of the language team's list. I keep hoping we can enhance this part of the type system some day.
I can open an SDK bug if you think it should be tracked over there.
Probably a good idea. I don't know when we'll have the resources to fix it, and I'd rather we didn't forget about it.
Activity
DanTup commentedon Aug 20, 2018
We're using the
elementDescriptionfor display here, though the result does have the correct type instaticType:@bwilkerson Is there value in showing
Objectto the user here? I could add thestaticTypein addition but it might be weird if the tooltip shows bothObjectandInstance. However, I can't (nicely) removeObjectbecause of it being embedded inelementDescription.bwilkerson commentedon Aug 20, 2018
It certainly makes sense to tell the user that the static type is "Instance" here, but there might be some value in also telling them how it was declared. Maybe it's largely a question of presentation. I don't know what the current hover text looks like, but perhaps something like this would make sense:
The static type is
Instance.It was declared as
Object object.(I say "like this" because I don't think the above is either attractive or readable.)
What I'd really like us to be able to display is the information to explain the difference in these two types (something like "because of the 'is' check on line lineNumber").
But it's possible that we ought to make the declared type and the element name be two different fields for ease of composition. (We have yet to find the sweet spot between making it flexible by having lots of fields and reducing the work on the client side by pre-formatting the output.)
DanTup commentedon Aug 20, 2018
Yeah, this comes up now and I was reminded of it when I saw this.
The tooltip for this currently looks kinda sparse, because it's a local variable with no docs or anything:
Yeah, that'd be neat. I'd love to show it as something like this:
I think putting the static type first makes sense since it's probably the one the user cares most about here, but with a note about the declared type and why it's different.
Looking in our hover code, I noticed we have something called the
propagatedType:I'm not sure I remember every seeing that rendered, so not sure what it is (the server docs just say it's the propagated type, but I'm none the wiser). For now, I'm gonna stick the static type on the end in parens:
Object object (Instance)And I'm moving propagated type to the same (since where it's currently rendered the label is fixed-with and looks wonky if I force it to show). Hopefully both won't appear at the same time as that'll be a little confusing!
DanTup commentedon Aug 20, 2018
Ugh, the static type is provided even when it matches the start of
elementDescriptionwhich means I end up withString a (String)on a normal string. In order to show the static type (and not look redundant), I'd need to know whether it's different, and I don't think I can do that reliably (I could check it starts with the static type then a space, but I think that might not be reliable since there could be spaces in function types?).I think this may need a server change before we can really improve it.
bwilkerson commentedon Aug 20, 2018
As of Dart 2, there shouldn't be any propagated type information (I need to deprecate the field). The propagated type was an early, analyzer-only, attempt to have better type information than the spec allowed for at that time. Now that we have type inference, we no longer compute a propagated type.
I can think of three ways to resolve that problem, two of which I'd already mentioned. First, we could split the
elementDescriptioninto adeclaredTypeandnamepair. Second, we could provide a singledescriptionfield that combined all of the information for clients so that they don't need to compose it for themselves. Third, we could omit thestaticTypefield when there's no useful information.We need to think about cases where there is no declared type (such as
var x = ...), cases like the one above where the declared and static type are significantly different (ObjectvsInstance), and cases where the static type is just a refinement (ListvsList<String>). And, of course, we shouldn't forget the cases where the variable isn't declared at all.DanTup commentedon Aug 20, 2018
Ah, makes sense (and why I haven't seen it lately).
I don't have any strong preferences on this. Having separate fields is more flexible, but I also like that giving us formatted strings gives a consistency across tools (and also I think people working on the server have a much better understanding of Dart than I, so are likely to format things better than me mashing a bunch of fields together). Any implementation that allows the user to see this extra info - which is a really neat feature of Dart (though it slightly bugs me that it doesn't work if in invert the condition - not sure if that's a bug?) - would be great. I can open an SDK bug if you think it should be tracked over there.
bwilkerson commentedon Aug 20, 2018
You and a lot of people :-). There was an effort started to enhance the flow of type information through code structures, but it keeps not being at the top of the language team's list. I keep hoping we can enhance this part of the type system some day.
Probably a good idea. I don't know when we'll have the resources to fix it, and I'd rather we didn't forget about it.
DanTup commentedon Aug 20, 2018
I've opened dart-lang/sdk#34196 with some notes from this issue.
[-]Tooltip doesn't show promoted type[/-][+]Tooltip doesn't show static type[/+][-]Tooltip doesn't show static type[/-][+]Show static types in tooltips[/+][analysis_server] Include static type in LSP hovers
DanTup commentedon Jan 19, 2022
Fixed by dart-lang/sdk@5bf643b.