Many false compatibilities: weak checks on interface members

[colin-grant-work]

When checking the compatibility of declared interfaces (for example InputBoxOptions), the current code in the comparator checks only for a match with an identical name: other metadata is ignored. For example, when comparing Theia master (0c0f8c7a7aedd67930bf65d6f63d827721707d60) with VSCode main (ef65649326cff17aae488a4fe953f0f4a6d70260), InputBoxOptions.validateInput is marked as compatible even though the declarations differ considerably:

Theia:

validateInput?: (input: string) => Promise<string | null | undefined> | undefined;

VSCode:

validateInput?(value: string): string | InputBoxValidationMessage | undefined | null |
			Thenable<string | InputBoxValidationMessage | undefined | null>;

Where InputBoxValidationMessage (which is marked as missing from Theia) is defined as

	/**
	 * Object to configure the behavior of the validation message.
	 */
	export interface InputBoxValidationMessage {
		/**
		 * The validation message to display.
		 */
		readonly message: string;

		/**
		 * The severity of the validation message.
		 * NOTE: When using `InputBoxValidationSeverity.Error`, the user will not be allowed to accept (hit ENTER) the input.
		 * `Info` and `Warning` will still allow the InputBox to accept the input.
		 */
		readonly severity: InputBoxValidationSeverity;
	}

The problem occurs here:

vscode-theia-comparator/src/comparator.ts

Lines 286 to 301 in 572e711

	if (docEntryLatestVsCodeCommand.members && docEntryLatestVsCodeCommand.members.length > 0) {
	// ok so here search in all members if it's defined in each vscode version
	docEntryLatestVsCodeCommand.members.forEach(member => {
	const searchedMember = inCurrent.members?.find(currentMember =>
	(currentMember.name === member.name));
	// it's there, add it
	if (searchedMember) {
	member.includedIn.unshift({ version: `theia/${theiaEntry.version}`, available: 'yes' });
	} else {
	member.includedIn.unshift({ version: `theia/${theiaEntry.version}`, available: 'no' });
	}
	});
	}

When checking for compatibility among members of interfaces, the only check performed is on the name field (l. 290). All other metadata is ignored.

[colin-grant-work]

@planger, I have created a branch where this problem is addressed by using internal TypeScript API to check types. I'm hesitant to make a PR because it requires modifying the TS source code in a postInstall hook to expose a method that is otherwise not available publicly on the TypeChecker interface (and because it's preliminary - I haven't integrated any of the data from the infos.yml file), but it seems to work reasonably well and reveals quite a number of cases in which TS does not believe that Theia's declarations match VSCode's.

I want to pull @alvsan09 into the discussion as well: like his PR, this adds a third category of warning to the dashboard. The question I want to pose is the purpose of this repo: is it an internal tool for planning development efforts on the Theia project, or is it a tool for plugin developers interested in ensuring that their plugins will work in particular versions of Theia? In the former case, the value of metadata is fairly limited, since the goal of development on Theia is ensure compatibility where possible, and the degree of compatibility in the past is of limited interest. In the latter case, accurate metadata is important, since it can provide more detailed information about real compatibility. In either case, the accuracy of the actual comparison is important, but the two approaches differ in the value of the infos.yml file. If we intend to provide accurate guidance for plugin developers, then the infos.yml and PR's like #35 are important; if we are primarily interested in the report as a planning tool, then infos.yml is not as important, and the information that #35 adds could just as well be provided by modifying the declarations in theia.d.ts to reflect the fact that the actual implementation does not provide the functionality the current declaration promises. For my part, I think the maintenance of the infos.yml is a bit cumbersome, so I would be inclined to avoid heavier reliance on its contents, but there may be an audience of Theia-oriented plugin developers for whom precise details of compatibility are important.

[JonasHelming]

@colin-grant-work : Thank you for raising this. IMHO, the report should actually fulfill both purposes. The idea is actually to make our planing visible and transparent and thereby allow plugin developers to also consume the report.
In practice, plugin developers would probably rather not use the report first but just try their plugin or directly develop against the available API. If they find an incompatibility, they can use the report to check whether there is already an issue and whether somebody is already working on it. Therefore, I see value in the infos.yml meta data as it is.
The same is true for the "stubbed" information, this one is even more important to have visible somewhere. Ite tells adopters that there are no current plans to really provide a specific API.
We also use the infos.yml for planing: You can compare with the target version and then create an issue for everything missing. During this process, we create the necessary meta data anyways, so we can also just add it to the infos.yml insetad to some private Excel file.

So in a nutshell,

• I am personally pretty happy with the report as is
• I think the "stubbed" state is valuable
• Improving the accuracy of the compatibility as you suggested would be highly appreciated

What we should revisit is the version columns. For Theia, we can probaly use "main" "last release" and "last community release" for VS Code "main" "last release" and "current compatibility target"

[alvsan09]

I had an offline discussion with @colin-grant-work and my understanding on the alternative solution to visualize stubbed API,
would work as follows:

Provided that the new more accurate type checker is in place

• A stubbed API can be implemented in a compatible way so extensions would be able to refer to them,
  however the implementation should not be an exact match so the new more accurate type checker can detect a mismatch in the structure,
  with the above, a matching name but mismatched structure can be resolved asstubbed.

The above alternative has several clear advantages as the stubbing is automatically detected and inserted in the actual source.

So given the not ideal way of forcing typescript to expose internal API,
What alternatives do we have to move this forward?

• Keep and maintain the forcing mechanism ?
• Develop our own ?
• Any other options ?

[JonasHelming]

Question about this mismatched structure: How do we differentiate between "mismatched structure but implemented" and "mismatched structure because subbed"?
We currently have one topic that we want to stub, i.e. Notebook support. Is the current overhead of adding this to the infos.yml worth optimizing this with a solution like the "mismatched structure" idea?

[colin-grant-work]

At the moment, the approach I'm taking poses a simple boolean question to TypeScript: whether the type of a certain declaration in Theia is identical to the corresponding declaration in VSCode. However, TypeScript capable of answering more detailed questions, and the old parser infrastructure shows one way - we can extract a lot of textual detail from declarations, so if we want to annotate things in theia.d.ts, there's a lot of scope for that, for example adding something like @stubbed to the JSDoc or StubbedX type aliases. I'd be in favor of keeping the documentation closer to the source in theia.d.ts rather than trying to keep metadata in a file like infos.yml up to date, but there are tradeoffs, more complex parsing here to extract the textual data among them, and remembering to delete the stubbed marker (whatever it might be) when we actually implement something.

[JonasHelming]

@colin-grant-work : I thing a marker such as @stubbed (or similar) is a great suggestion and for me a perfect solution. It documents the "stubbed" in code and we can automatically keep the comparator report up to date. I do not see an issue with forgetting to remove the tag when implementing something. If somebody forgets, we will identify that in the report still.
I would keep the infos.yml for linking tickets then, as it is currently used.

[JonasHelming]

@planger

[planger]

@colin-grant-work Sorry for chiming in late!

First of all, thank you for this great work so far! Imho it is excellent to reuse TypeScript's own evaluation to decide whether declarations match or not instead of relying on an own set of extracted metadata.

@planger, I have created a branch where this problem is addressed by using internal TypeScript API to check types. I'm hesitant to make a PR because it requires modifying the TS source code in a postInstall hook to expose a method that is otherwise not available publicly on the TypeChecker interface (and because it's preliminary - I haven't integrated any of the data from the infos.yml file), but it seems to work reasonably well and reveals quite a number of cases in which TS does not believe that Theia's declarations match VSCode's.

I wouldn't be opposed to that. It may not be elegant, but I wouldn't expect it to cause a lot of maintenance work for us in the future, as we are not changing TypeScript version all too often. Or do you think I'm misjudging that?
In addition we could open a PR against TypeScript to make this method visible, explain why we need it, and see if it gets in?

is it an internal tool for planning development efforts on the Theia project, or is it a tool for plugin developers interested in ensuring that their plugins will work in particular versions of Theia?

I believe it is very useful for both purposes. I guess, however, currently it is more used for planning the development efforts and tracking progress, rather than ensuring a certain plugin will work. It makes clear what is missing for which VS Code version, whether missing API is already tracked, and by following the link to the issue what is already being worked on. E.g. for me it was the basis for creating eclipse-theia/theia#11520. But I think it is also relevant to adopters in planning with which Theia versions they can integrate certain VS Code extensions (third-party ones or own development).

For the use case of testing compatibility or ensuring it during development of a VS Code extension, I find https://github.com/eclipse-theia/theia-vscodecov very interesting. It might not be ready yet (see https://github.com/eclipse-theia/theia-vscodecov/issues?q=is%3Aissue+is%3Aopen+bug%3A), but it addresses this use case more directly, as otherwise you'd have to know which API an extension actually uses and then manually match it against the report we generate with vscode-theia-comparator, which seems impracticable anyway.

So maybe it is worth optimizing theia-vscodecov for testing and ensuring compatibility of a specific VS Code plugin and vscode-theia-comparator for planning and tracking progress of compatibility in general?

I'd be in favor of keeping the documentation closer to the source in theia.d.ts rather than trying to keep metadata in a file like infos.yml up to date, but there are tradeoffs, more complex parsing here to extract the textual data among them, and remembering to delete the stubbed marker (whatever it might be) when we actually implement something.

Yes, I agree. Adding a @stubbed marker to the actual source code is a great idea and don't think it'll be a problem that we forget to remove it. Typically someone changes code to establish compatibility anyway and then this marker should be removed. If we forget, we'll stumble upon it in the report.

I also don't particularly enjoy editing the infos.yml ;-). But I think having the links to the Github issues alongside the missing API is very useful. At least for seeing clearly whether there is missing API that is not even tracked yet. For this type of information, however, it doesn't feel practical to be kept in the theia.d.ts:
Adding issue numbers for API that is not even stubbed yet, changing issue numbers because it is split into two work streams, etc. Also this information feels closer to the reporting rather than closer to the Theia source. So for this purpose I guess it makes more sense to keep the infos.yml.
However, I'm not opposed to adding this information in the theia.d.ts too, e.g. copying over the type declaration and adding @stubbed(#1234), because once, compatibility is established, I don't think this information is relevant anymore.

I'll give #36 a try to today or tomorrow! I'm excited to see these improvements! Thanks!

[planger]

For the use case of testing compatibility or ensuring it during development of a VS Code extension, I find https://github.com/eclipse-theia/theia-vscodecov very interesting. It might not be ready yet (see https://github.com/eclipse-theia/theia-vscodecov/issues?q=is%3Aissue+is%3Aopen+bug%3A), but it addresses this use case more directly, as otherwise you'd have to know which API an extension actually uses and then manually match it against the report we generate with vscode-theia-comparator, which seems impracticable anyway.

One more thought:
We may want to at some point provide tooling to easily allow building a VS Code extension against theia.d.ts in addition or instead of vscode.d.ts. This way it'd be more convenient for VS Code extension authors to ensure compatibility with both, VS Code and Theia.

[alvsan09]

I also think that having a decorator like @stubbed would be much better than maintaining additional metadata in the infos.yml,
I will gladly look into it and replace #35 accordingly.

@colin-grant-work, during your investigation of the identical matching, did you see any API from typescript that would be useful for this purpose?

[colin-grant-work]

Resolved by #36