Systems-Modeling / SysML-v2-Pilot-Implementation

Proof-of-concept pilot implementation of the SysML v2 textual notation and visualization
GNU Lesser General Public License v3.0
114 stars 23 forks source link

ST6RI-754 Eliminate the KerMLDerivedStateComputer #562

Closed seidewitz closed 2 months ago

seidewitz commented 2 months ago

Background

The KerMLDerivedStateComputer was introduced as part of the changes in PR #177 (commit https://github.com/Systems-Modeling/SysML-v2-Pilot-Implementation/pull/177/commits/f757986f0900e32d1a0f73e5ec00b153a088c2be). It implements the Xtext DerivedStateComputer mechanism such that resources opened as DerivedStateAwareResources automatically have ElementUtil::transformAll called on them to install their "derived state". The use of the KerMLDerivedStateComputer thus ensured that all the adapter transformations are called on all loaded resources before validation.

A disadvantage of the this mechanism is that, when a KerML or SysML file is opened in the Eclipse editor, the KerMLDerivedStateComputeris also called for other resources loaded because of elements in them referenced from the file being opened. This then generally results in proxy resolutions that can cause yet further resources to be loaded, until an effective transitive closure of all directly and indirectly referenced resources are loaded. As a consequence, it can take a noticeable amount of processing time for a file to open in Eclipse, especially of the file references a complex model library like Quantities and Units. In addition, DerivedStateAwareResource has discouraged access outside of the Xtext base implementation, and, thus, the mechanism could be changed or eliminated due to future Xtext implementation decisions.

As the implementation has evolved, the essential computation of implicit types is now being done “on demand” during name resolution, rather than relying on the derived state computer. The main remaining need for the derived state computer is for a few cases in which elements are still being physically inserted into the abstract syntax tree, rather than being stored implicitly in adapters. However, the recent update in PR #552 has introduced a new mechanism that allows such element insertions to generally be handled independently of the KerMLDerivedStateComputer.

Update

This PR updates the implementation so that the KerMLDerivedStateComputer is no longer necessary. It includes the following changes.

  1. Eliminates the KerMLDerivedStateComputer and removes the binding of XtextResource to DerivedStateAwareResource in KerMLRuntimeModule.
  2. Replaces the used of DerivedStateAwareResourceValidator with a KerMLResourceValidator that calls ElementUtil::transformAll before resolving proxies during validation.
  3. Adds KerMLLazyLinkingResource to allow unresolvable URI fragments to be cleared after transformation, so that they can potentially be subsequently resolved after all.
  4. Updates KerMLOutlineTreeProvider to ensure that an element is transformed before being shown, even if validation has not been done yet.
  5. Fixes unintended dependencies on the derived state computation.
    • Moves setting of bound value result for a Feature to addDefaultGeneralType.
    • Removes the transformation of ends on getRelatedFeatureOfEnd.
    • Revises getAllFeaturingTypes of a Feature to handle FeatureValues correctly even if transformation has not been done.
    • Fixes transformAnnotatingElement in AnnotatingElementAdapter.

Consequences

As a result of the changes in this PR, KerML and SysML files can open without delay in the Eclipse editor. However, the performance of subsequent validation and name resolution is not improved and is essentially the same as before the changes.

himi commented 2 months ago

Ed, thank you for the detailed explanation. While this will make a considerable impact, the changes made here look straightforward. I'll run and check it for testing.

himi commented 2 months ago

I'm wondering why we need to clear unresolvable URI fragments in KerMLLazyLinkingResource after the transformation. Rather I'd thought we should keep them to know unresolved links. Maybe I'm missing something and we need to clear it before validation.

At least I think we should add copyright notice comment to KerMLLazyLinkingResource, or we should remove it and just by calling getUnresolvableURIFragments().clear() if the resource is an instance of LazyLinkingResource.

seidewitz commented 2 months ago

@ujhelyiz You are right, DerivedStateComputer is not actually "deprecated" as such. Rather, it is marked as "discouraged access" from its Xbase project, so it is not really intended to be used outside the Xtext implementation. I can clarify this in the PR description.

seidewitz commented 2 months ago

@himi

I'm wondering why we need to clear unresolvable URI fragments in KerMLLazyLinkingResource after the transformation.

Because of the way our name resolution algorithm gets recursively called due to on-demand proxy resolutions, sometimes proxies get marked as "unresolvable" simply because we have put them on the list of "already visited" members in the midst of name resolution. However, once a proxy is marked as unresolvable, Xtext, by default, will not try to resolve it again, even though in a number of cases it could ultimately be resolved as the name resolution process unwinds. This has been an ongoing problem with getting name resolution to work right in some edge cases.

By running the transformation first, enough proxies get resolved to complete, e.g., all the implicit specialization computations. Clearing the "unresolved proxies" after the transformations means that there is a second chance to resolve them during the standard proxy resolution pass, given the resolutions that were successful during the transformation phase. If a previously unresolved proxy gets resolved, then the previous error message for it is removed from the diagnostic list. Otherwise, the error remains and is reported as usual.

During testing, I confirmed that this was, indeed, necessary, because some Xpect tests do not pass without clearing the unresolved proxies before the final proxy resolution. I tried several different ways of handling this, and the approach proposed in this PR is the simplest I could find that allows all tests to pass and all library and example models to parse without errors or warnings.

At least I think we should add copyright notice comment to KerMLLazyLinkingResource

You are right, I will add that.

we should remove it and just by calling getUnresolvableURIFragments().clear() if the resource is an instance of LazyLinkingResource

getUnresolvableURIFragments only has protected visibility in LazyLinkingResource. Since all that we needed to do was to be able to clear the collection, I just added the public clearUnresolvableURIFragments method to allow this, rather than overriding getUnresolvableURIFragments to make it public.

himi commented 2 months ago

Ed, thank you for the detailed explanation. I could understand the situation. Actually, I did not notice getUnresolvableURIFragments() is protected.