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

Direct comparison via TypeScript's isTypeAssignableTo function #36

Merged

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Aug 17, 2022

This PR proposes an alternative strategy for comparing theia.d.ts and vscode.d.ts. Currently, metadata is extracted from the declaration files, and then that metadata is compared at varying levels of detail. In this experiment, the internal typechecking function isTypeIdenticalTo, which is present on TypeChecker but not declared, is used to answer the question of compatibility directly, so that every member of the declaration can be labeled present and compatible, present but not compatible, or absent.

This PR also makes a number of changes to the report:

  • Full and filtered reports are presented in a single page with a toggle.
  • Present but not identical declarations are presented in 'warning' yellow.
  • The top row of the table is sticky so that it is always visible.
  • The report is presented as an HTML table for more consistent spacing and easier pasting into other contexts.
  • Emoji are used in place of Font Awesome icons, again to facilitate pasting into other contexts.

Screen Shot 2022-08-29 at 12 25 33 PM

Screen Shot 2022-08-29 at 12 44 45 PM

Todo:
This is currently just a proposal and a demonstration, and it should be evaluated thoroughly before acceptance. There are a few known todos:

  • There was previously special handling for certain namespaces (e.g. extensions). I've removed that for now, since it isn't clear to me why we shouldn't match those in theia.d.ts instead of having special handling here.

Advantages:

  • The code for parsing and comparison is simplified considerably.
  • The standard of comparison is pretty strict, so false reports of compatibility are unlikely. There is a stricter check isTypeIdenticalTo available internally to TypeScript, but for example it considers even code-identical enum declarations not to be identical, so it may be too strict.
  • More authoritative comparison: if this works as designed, we get TypeScript's own evaluation of whether our declarations match, rather than a comparison of select pieces of metadata.

Disadvantages:

  • Knock-on incompatibility: in the current iteration, non-identical declarations have a knock-on effect: if a given interface is declared incompatible, then any other declaration that refers to it is also (likely to be) considered incompatible.

This has been addressed by falling back to a string comparison of the declarations. If the declarations are identical, then the incompatibility is assumed to lie elsewhere, so the field is marked as compatible.

  • Loss of metadata: at the moment, isTypeAssignableTo is used as a black box: we get no information about why declarations don't match. There may be other API that would provide more information (e.g. checkTypeRelatedTo). On master the parsing method produces more metadata and can be inspector more easily for problems.

@colin-grant-work colin-grant-work changed the title Direct comparison via TypeScript's isTypeIdenticalTo function Direct comparison via TypeScript's isTypeAssignableTo function Aug 17, 2022
@JonasHelming
Copy link
Contributor

Just a quick feedback, I am heading out for vacation:

I totally like the HTML changes (Table, filter, etc.)

About the comparison: It would be very beneficial to have a more precise comparison. However, we should have a close look at "present but not compatible". If these entries are actually compatible in practice, i.e. an extension developer can use the API without noticing a thing, then the report would show a lot of false positives.
This would be bad because it would give the impression to adopters that a lot is missing which is actually there and working. Also we could not use the report to focus on the things that a really missing anymore.

@planger

@planger
Copy link
Contributor

planger commented Aug 18, 2022

@colin-grant-work Great work! 👍 I've tested this PR and looked at the code, for now just on a high level. Overall it makes a very good impression and I really like the improved report and certainly find the increased precision to be very valuable!

Of course, this new approach for a compatibility check makes a lot of green lines orange, which doesn't look great. However, I guess at this stage, it is hard to tell which of the orange ones actually do have a larger impact or which don't. Anyway, better we know it and work on it rather than sweeping it under the rug.

Until we cleaned up the easily fixable incompatibilities, however, we may want to just show the warning in a less aggressive way, e.g. say "partly supported" or ✔️~, to avoid scaring off adopters from using certain plugins or even Theia in the first place, even though many of the plugins using that API would actually work. In fact, we currently don't know how impactful those inconsistencies are.

But eventually we should, imho, aim for 1:1 equivalent definitions and rather work around technically reasonable differences or partly unsupported behavior in the plugin-ext implementations. I may miss something, but I can't really think of a good reason to keep differences in theia.d.ts vs vscode.d.ts.

@colin-grant-work What's your overall feeling on the newly raised incompatible ones? Are they mostly minor "structural" but less semantic differences?

For instance, the example @colin-grant-work brought up in #32 (comment) is a clear incompatibility and should be addressed. If someone puts in an InputBoxValidationMessage, chances are we either face a runtime error, because someone calls a property/method on it that is only available for a string, or we output the InputBoxValidationMessage as object directly to the UI.
Both is bad but could be fixed quite easily, in the worst case by just forward input.message rather than just input if it is a InputBoxValidationMessage in plugin-ext, also if that means we skip supporting InputBoxValidationSeverity for now (which we actually seem to already support somewhere).

I could offer to skim the incompatibilities a bit next week, if that helps. If I'm not mistaken, there are about 200 of them though ;-) which leads to the next point:

Report-wise, imho the most important issue is the knock-on effect, as it kind-of distorts the report and doesn't clearly point to the actual culprit anymore. Do you think it would be feasible to address this effect with reasonable effort, e.g. with a second pass to check the incompatible ones more locally?
I understand that it is technically correct that, if B is incompatible, also a(b: B) is incompatible. So callers of a(b: B) might already face issues. But marking a and B with the same level of incompatibility makes the report less precise.

I didn't find the place where you use isTypeIdenticalTo and modified the TypeScript code? Is that part of the PR?

@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Aug 18, 2022

If these entries are actually compatible in practice, i.e. an extension developer can use the API without noticing a thing, then the report would show a lot of false positives. This would be bad because it would give the impression to adopters that a lot is missing which is actually there and working. Also we could not use the report to focus on the things that a really missing anymore.

Until we cleaned up the easily fixable incompatibilities, however, we may want to just show the warning in a less aggressive way, e.g. say "partly supported" or ✔️~.

I can't really think of a good reason to keep differences in theia.d.ts vs vscode.d.ts.

@planger @JonasHelming, thanks for your feedback. I've taken a look (with @vince-fugnitto) at some of the new incompatibilities. Some of them are pretty trivial: a lot of missing readonly modifiers, some cases of field?: T as opposed to field: T | undefined. This PR reduces the incompatibility count by 115 by fixing many of those issues. There are likely others I've missed, because I focused on cases where isTypeAssignableTo returned true in one direction (usually Theia -> VSCode) but not the other (since e.g. mutable T[] is always assignable to readonly T[]).

A lot of the other new compatibility issues are knockons. A good example there is that Terminal is not fully compatible, because Terminal.exitStatus in Theia is missing a reason field. As a consequence, almost every field that refers to Terminal (TerminalLinkContext, window.terminals, window.activeTerminal, window.onDidChangeActiveTerminal, window.onDidOpenTerminal, window.createTerminal etc.) is marked as only partially compatible. For extension developers, that makes some sense: it's indirect, but it may worth knowing that using those methods may bring in some unexpected behavior; for us, the derived incompatibilities aren't very useful, since we have to do the detective work of tracking down the real reason behind them all.

Given that, I think @planger's suggestion that we at a minimum reduce the apparent severity of the warning is a good one to give us time to address straightforward cases and reduce the number of derived incompatibilities we see in mostly-supported API. It may also make sense to specify the target VSCode version explicitly, and either omit main in the published report or mark missing API that postdates our current target less agressively. But eventually, I agree completely with the idea that theia.d.ts should be identical to vscode.d.ts. In the few cases where we have enhancements beyond VSCode's capability, we can declare them separately in a merged declaration.

Do you think it would be feasible to address this effect with reasonable effort, e.g. with a second pass to check the incompatible ones more locally?

It should be possible to set things up so that if an incompatibility is detected, we check some other heuristic - simplest would be string identity of the nodes - and if it passes that, then we assume that the fault lies somewhere else. I'll experiment with that.

I didn't find the place where you use isTypeIdenticalTo and modified the TypeScript code? Is that part of the PR?

I've switched to using isTypeAssignableTo instead of isTypeIdenticalTo. For one thing, it was already present on the TypeChecker, so it doesn't require postInstall modification of the TS source (although it's undeclared, so it may disappear unexpectedly), and because isTypeIdenticalTo turned out to be too strict: even literally identical enums would be declared incompatible.

@colin-grant-work
Copy link
Contributor Author

It should be possible to set things up so that if an incompatibility is detected, we check some other heuristic - simplest would be string identity of the nodes - and if it passes that, then we assume that the fault lies somewhere else. I'll experiment with that.

A check for string identity of the declarations reduces incompatibilities by about 90, it appears. I'll push a patch soon.

@colin-grant-work
Copy link
Contributor Author

I've implemented the check for string identity of the declarations. That reduces the reported incompatibilities on Theia master by 176 (784-608) and this PR reduces them by a further 90.

@colin-grant-work
Copy link
Contributor Author

I've changed the 'warning' to have a green background with a yellow circle. I'm open to suggestions - @vince-fugnitto?

image

@vince-fugnitto
Copy link
Member

I've changed the 'warning' to have a green background with a yellow circle. I'm open to suggestions - @vince-fugnitto?

@colin-grant-work I have the following suggestions if interested:

  • removing the blue background as it serves no real purpose
  • I like the border it makes navigating the table much easier, one improvement would be to add a hover highlight
  • I would maybe replace the icons with a label for searchability and clarity
  • I would maybe make the colors lighter with more focus on the content
  • I would keep 3 levels of VS Code columns (main, a target api, and the current api)

I have the following POC to show my feedback:

comparator.mov

@paul-marechal
Copy link
Member

I would keep 3 levels of VS Code columns (main, a target api, and the current api)

Maybe I'm missing something, but what's the point of the VS Code - Main column? Wouldn't it always be full of ticks, meaning the info is not super useful?

@planger
Copy link
Contributor

planger commented Aug 26, 2022

@vince-fugnitto Looks awesome! Thanks! I'd just have a minor suggestion: I think it could be better to make partial and stubbed visually different from each other. E.g. partial has a green background but the "button" is still orange?

@alvsan09
Copy link

I would keep 3 levels of VS Code columns (main, a target api, and the current api)

Maybe I'm missing something, but what's the point of the VS Code - Main column? Wouldn't it always be full of ticks, meaning the info is not super useful?

I agree, I think the column API and VS Code - Main could be merged

@vince-fugnitto
Copy link
Member

I updated it slightly:

  • each column has some searchable content (apis not present in a specific vscode version are greyed out)
  • partial support is marked as a warning since it requires our attention but may work
  • stubbed apis are greyed out since they are not supported by choice

Screen Shot 2022-08-26 at 11 15 52 AM

@colin-grant-work
Copy link
Contributor Author

@planger, where you do stand on the current state of this PR? I think the TypeScript comparison is better than the existing comparison, and I think that with my PR, there are basically no false reports of incompatibility: in all of the cases where TS reports that a field is incompatible, inspection confirms that there is a substantive difference in the declaration. We will have to be vigilant about unresolved references leading to any fallbacks in theia.d.ts - ideally we could put it somewhere where we could turn off skipLibCheck - but otherwise, I don't see much downside to making the switch and getting to work resolving the conflicts discovered. I'd be happy to wait, though, until after my PR on the Theia side is merged (running into possible problems with the typedoc generator - I think otherwise it's good to go).

Copy link

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

It is looking very good 👍

I have tested the new solution with a focus on the presentation of stubbed,

The testing was performed against stubbed theia.d.ts versions available using:
THEIA_URL_PATTERN = 'https://raw.githubusercontent.com/alvsan09/test_theia.d.ts/${VERSION}/theia.d.ts';

The expected results mentioned below correspond to the mocked theia.d.ts mentioned above.
I noticed the following:

  • Type decorators are missing, I think they are quite useful
  • Type aliases are being incorrectly shown as members of a previous entry
    e.g. Definition, DefinitionLink
  • Stubbed annotation missing for some entries e.g.
    EvaluatableExpression v1.29.0, TreeItem master
  • Extension does not reflect any implementations, however the former solution
    seem to somehow map the status to an equivalent Plugin entry, so this mapping is missing.
  • Enum's reflected as child entries, e.g. InputBoxValidationSeverity
  • From above, Enum members are not shown
  • Memento#keys show Unsupported for all versions, however the entry is present from v1.29.0
  • Memento#keys update is expected to show stubbed for the master version.
  • Multiple signatures of function showQuickPick show as only a single entry

@@ -12,7 +12,14 @@
import * as ts from 'typescript';
import { RecursiveRecord } from './recursive-record';

export interface Comparison extends RecursiveRecord<boolean | null> { }
export const enum SupportLevels {

Choose a reason for hiding this comment

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

Minor:
Although not explicitly stated on the theia coding conventions,
most enum names are singular.

Copy link

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

Updating my vote, as per previous review ...

@planger
Copy link
Contributor

planger commented Aug 29, 2022

@colin-grant-work I'm much in favor of this PR! The enhanced precision of this report surely will help us identifying where we need to improve and I certainly appreciate the improved style of the report. 👍 I guess waiting for your PR to fix the "pseudo" incompatibilities would help avoiding a time period where the report is a bit overly pessimistic.

Thank you @alvsan09 for making the comparison with the old report! I had that on my todo list for tonight.

Once those are fixed, I'm really looking forward to this PR getting merged. Thanks a lot!

@colin-grant-work
Copy link
Contributor Author

@alvsan09, thanks for taking a thorough look through the results. I will see what adjustments need to be made 👍

@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Aug 29, 2022

  • Type decorators are missing, I think they are quite useful

This is noted in the description under the heading 'loss of metadata.' I don't find this data very helpful: an extension developer who wants to use the API likely knows what's a function and what's a class; a developer improving the API will have to look at the declarations anyway. But they can be restored if there's consensus about their inclusion


  • Type aliases are being incorrectly shown as members of a previous entry
    e.g. Definition, DefinitionLink
  • Enum's reflected as child entries, e.g. InputBoxValidationSeverity

Fixed - they're now displayed as top-level.


  • Stubbed annotation missing for some entries e.g.
    EvaluatableExpression v1.29.0, TreeItem master

At the moment, the code expects @stubbed to appear on the level of members, not complex entities, and then derives the state of the complex from the state of the members. But it could be adjusted to accept annotations on the complex entities.


  • Extension does not reflect any implementations, however the former solution
    seem to somehow map the status to an equivalent Plugin entry, so this mapping is missing.

Also noted in the description:

There was previously special handling for certain namespaces (e.g. extensions). I've removed that for now, since it isn't clear to me why we shouldn't match those in theia.d.ts instead of having special handling here.

In particular, it is not correct to say that we support the extensions namespace if you cannot in fact write vscode.extensions and get the correct result in Theia.


  • From above, Enum members are not shown

This is deliberate - enums are compared as simplices rather than complices. It would be possible to extract the keys, if desired. (I've made this change).


  • Memento#keys show Unsupported for all versions, however the entry is present from v1.29.0

This is actually correct: theia.d.ts apparently declares Memento twice in the global namespace.

https://github.com/eclipse-theia/theia/blob/da356b537037680cdc7283a695ebfbf0c00d2cf1/packages/plugin/src/theia.d.ts#L3492-L3530

https://github.com/eclipse-theia/theia/blob/da356b537037680cdc7283a695ebfbf0c00d2cf1/packages/plugin/src/theia.d.ts#L11224-L11255

and the double declaration makes it incorrect.


  • Multiple signatures of function showQuickPick show as only a single entry

This is a consequence of the comparison method - it's simpler to allow TS to compare the effective constraint rather than attempting to match up overrides.

@alvsan09
Copy link

  • Type decorators are missing, I think they are quite useful

This is noted in the description under the heading 'loss of metadata.' I don't find this data very helpful: an extension developer who wants to use the API likely knows what's a function and what's a class; a developer improving the API will have to look at the declarations anyway. But they can be restored if there's consensus about their inclusion

I would prefer to reinstate them and save the space from other columns e.g. by decreasing the number of vscode columns as Paul suggested above.
Not a blocker though!

@alvsan09
Copy link

I would prefer to reinstate them and save the space from other columns e.g. by decreasing the number of vscode columns as Paul suggested above. Not a blocker though!

I was chatting off-line with @vince-fugnitto and he has raised a valid concern on the icons being used by the former solution e.g. Copyright for Classes, forsquare for functions, and venmo for variables, that would definitely need to be improved, however it would make sense to address it in a separate PR later in the future and if there is a consensus to bring them back.

As per the rest of the explanations, I am fine with them!
Thanks @colin-grant-work !

Copy link

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

I have used showQuickPick as a reference to test the last two commits and they work fine 👍
The code looks good to me as well !

@colin-grant-work
Copy link
Contributor Author

I'll merge tomorrow unless there are objections (@planger, with a CC to Jonas, if he's not still on vacation).

@planger
Copy link
Contributor

planger commented Sep 2, 2022

@colin-grant-work Thanks for the heads-up! 👍 for merging. Jonas will be back next week.

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.

6 participants