Add isObject check to ProblemMarker
#12251
Conversation
| export function is(node: Marker<object>): node is ProblemMarker { | ||
| return 'kind' in node && node.kind === PROBLEM_KIND; | ||
| return isObject<ProblemMarker>(node) && node.kind === PROBLEM_KIND; | ||
| } |
There was a problem hiding this comment.
The fact that we have to use isObject here to determine that the node is an object implies that the Marker<object> contract doesn't hold up as expected. We should use node: unknown instead.
There was a problem hiding this comment.
Good point!
I updated the pr and changed the cast to Marker<object>
The `in` operator does not work on undefined objects. So in order to check the node kind we need to make sure, that the node is an object. Contributed on behalf of STMicroelectronics Fixes eclipse-theia#12250
| export function is(node: Marker<object>): node is ProblemMarker { | ||
| return 'kind' in node && node.kind === PROBLEM_KIND; | ||
| export function is(node: unknown): node is ProblemMarker { | ||
| return isObject<Marker<object>>(node) && node.kind === PROBLEM_KIND; |
There was a problem hiding this comment.
I'm not sure this is correct: shouldn't we assert it's a marker first? Right now an object like this wold fit the bill:
{
kind: 'problem'
}
would fit the bill, even though it's not a marker. But asserting ProblemMarker.is(foo) implies that it's a marker, typing-wise.
There was a problem hiding this comment.
@tsmaeder I agree, the code is very optimistic. But the previous code did the same assumption.
So I would suggest to tackle this improved marker assertion in a separate ticket.
There was a problem hiding this comment.
Well, during the runtime, yes. But from a static analysis perspective, it assumed that the node already is a Marker<object>. So we should confirm that as well.
|
Closing as a better fix will be provided by #12259 |
What it does
Use isObject check in problem-marker class instead of
'kind' in nodecheck to not get type errors which occur on non marker. These occur because a previously existing check was removed in problem-selection.ts . The error prevents the context menu from showing all entries.Contributed on behalf of STMicroelectronics
Fixes #12250
How to test
Collapse All, with the fix 3 entries are shown:Copy,Copy MessageandCollapse AllReview checklist
Reminder for reviewers