Layer violations in Structure101

[sglebs]

In the process of testing our plugin/flavour for Structure101/g which parses JS/TS projects, we identified a few problems with the layering in VSCode 08b92738094. In a private channel, @egamma asked us to enter this issue.

Steps to Reproduce:

1. Install Structure 101/g Studio
2. Install the flavour/plugin net.betterdeveloper.javascript.imports 1.1.2
3. Get the VSCode commit hash 08b92738094 (to make sure it is the same src I looked at)
4. File->New, choose the right flavour/plugin above, and run pointing at the VS Code src dir and the tsconfig file, like this:

When the conversion finishes, you will get some layer violations:

1. platform is using editor. "/platform/browser/contextScopedHistoryWidget.ts imports /editor/contrib/suggest/suggest.ts". This violation was reported in private and issue #140856 was entered by @egamma

   

2. bootstrap-amp.js uses "vs" and vice-versa:

   
   bootstrap-amd.js    imports vs/base/common/performance.js       
   bootstrap-amd.js    imports vs/loader.js        

vs/editor/test/common/model/benchmark/bootstrap.js imports bootstrap-amd.js
vs/server/cli.js imports bootstrap-amd.js
vs/server/main.js imports bootstrap-amd.js

<img width="301" alt="image" src="https://user-images.githubusercontent.com/1507221/149835341-c140df64-ecd3-48a5-aa20-27242807010f.png">

3. base uses platform: (although that seems due to the fact that test code is stored in the same folder as production code. We will send another analysis with synthetic model transformation to emulate tests in a separate dir tree)

/base/parts/ipc/test/node/ipc.net.test.ts imports /platform/product/common/product.ts
/base/test/parts/quickinput/browser/quickinput.test.ts imports /platform/list/browser/listService.ts

<img width="153" alt="image" src="https://user-images.githubusercontent.com/1507221/149835673-7997996c-c209-4a6e-883a-eed646c4c62f.png">

I will enable/explain Model Transformations and enter a separate comment for that mode (extracting tests out into a separate structure).

[sglebs]

In an attempt to simulate tests being separate from production code and see if production code uses test code or not, I made use of Model Transformations in Structure101:

  <transformations>
    <transformation in="*/test/*" out="[test]/{1}/test/{2}" />
    <transformation in="*/testing/*" out="[test]/{1}/testing/{2}" />
    <transformation in="*/*Testing.ts" out="[test]/{1}/{2}Testing.ts" />
    <transformation in="*/*.test.ts" out="[test]/{1}/{2}.test.ts" />
    <transformation in="*/*.integrationTest.ts" out="[test]/{1}/{2}.integrationTest.ts" />
    <transformation in="*/workbenchTestServices.ts" out="[test]/{1}/workbenchTestServices.ts" />
  </transformations>

The need for many rules comes from the multiple naming conventions present in the code.

There are cycles from vs into test even with the transformations above:

vs/workbench/api/browser/extensionHost.contribution.ts  imports [test]/vs/workbench/api/browser/mainThreadTesting.ts        
vs/workbench/api/common/extHost.api.impl.ts imports [test]/vs/workbench/api/common/extHostTesting.ts        
vs/workbench/api/common/extHost.protocol.ts imports [test]/vs/workbench/contrib/testing/common/testCollection.ts        
vs/workbench/api/common/extHostTestingPrivateApi.ts imports [test]/vs/workbench/contrib/testing/common/testId.ts        
vs/workbench/api/common/extHostTypeConverters.ts    imports [test]/vs/workbench/contrib/testing/common/testCollection.ts        
vs/workbench/api/common/extHostTypeConverters.ts    imports [test]/vs/workbench/contrib/testing/common/testId.ts        
vs/workbench/contrib/snippets/browser/snippetsService.ts    imports [test]/vs/editor/test/common/modes/testLanguageConfigurationService.ts      
vs/workbench/workbench.common.main.ts   imports [test]/vs/workbench/contrib/testing/browser/testing.contribution.ts     

Do we want to have sub-layering inside platform? Note that in platform there is also a small tangle between diagnostics and launch:

/launch/electron-main/launchMainService.ts  imports /diagnostics/common/diagnostics.ts      

and a much bigger tangle:

Workbench also shows a tangle:

[chedgers]

On @sglebs's prompting I took a look at the code in S101 Studio. I notice that the workbench folder contains an underlying file tangle of 749 files. I think you'd need to break that up a bit in order to layerize the workbench sub-folders. Removing just a couple of outgoing dependencies from workbench.web.main.ts would (if appropriate) break the big file tangle into much smaller ones (max 42 classes), which should make restructuring to a layered structure much easier.

I'd be happy to contribute if this is important to the team - let me know.

[bpasero]

Some feedback:

• contrib/testing is not a violation if you think this is test code. that is actually enabling the testing functionality in VSCode
• it is expected that workbench.web.api.ts depends on "everything" because this is the main entry file for VSCode for web
• I think we can exclude the bootstrapping JS files from layer breaks
• I think we do not have to look at dependencies from tests

[sglebs]

Regarding test code: IMHO tests should sit on top of code that enables testing, but not the other way around. In my second comment above I have used model transformations to simulate tests being in a separate root "namespace" ( you will notice that I added a synthetic /[test] folder/package at the top). It may be worth tweaking the rules I used, maybe I moved too much into /[test] (I would need a deeper knowledge of the codebase at this point). Anyway, here are the rules I used to move test code to an upper layer "/[test]":

  <transformations>
    <transformation in="*/test/*" out="[test]/{1}/test/{2}" />
    <transformation in="*/testing/*" out="[test]/{1}/testing/{2}" />
    <transformation in="*/*Testing.ts" out="[test]/{1}/{2}Testing.ts" />
    <transformation in="*/*.test.ts" out="[test]/{1}/{2}.test.ts" />
    <transformation in="*/*.integrationTest.ts" out="[test]/{1}/{2}.integrationTest.ts" />
    <transformation in="*/workbenchTestServices.ts" out="[test]/{1}/workbenchTestServices.ts" />
    <transformation in="*/extHostTestingPrivateApi.ts" out="[test]/{1}/extHostTestingPrivateApi.ts" />
  </transformations>

and here are the violations that remain according to the above logical layering:

vs/workbench/api/browser/extensionHost.contribution.ts  imports [test]/vs/workbench/api/browser/mainThreadTesting.ts        
vs/workbench/api/common/extHost.api.impl.ts imports [test]/vs/workbench/api/common/extHostTesting.ts        
vs/workbench/api/common/extHost.protocol.ts imports [test]/vs/workbench/contrib/testing/common/testCollection.ts        
vs/workbench/api/common/extHostCommands.ts  imports [test]/vs/workbench/api/common/extHostTestingPrivateApi.ts      
vs/workbench/api/common/extHostTypeConverters.ts    imports [test]/vs/workbench/api/common/extHostTestingPrivateApi.ts      
vs/workbench/api/common/extHostTypeConverters.ts    imports [test]/vs/workbench/contrib/testing/common/testCollection.ts        
vs/workbench/api/common/extHostTypeConverters.ts    imports [test]/vs/workbench/contrib/testing/common/testId.ts        
vs/workbench/contrib/snippets/browser/snippetsService.ts    imports [test]/vs/editor/test/common/modes/testLanguageConfigurationService.ts      
vs/workbench/workbench.common.main.ts   imports [test]/vs/workbench/contrib/testing/browser/testing.contribution.ts     

Probably some entries should not have been moved to the upper layer "/[test]" in my model transformations - this can be easily changed once it is confirmed/determined by an insider engineer/architect.

Another approach is to simply skip test from the analysis. I avoid this because I have seen my fair share of production code using test code in previous lives, so I like to see a clean layer structure where "/tests" uses code underneath but not the other way around. Model transformations are great because they allow this logical view & enforcement without imposing a strict folder structure on the teams.

Congrats on the great work, let me know if I can be of any further assistance. I can easily run the analysis again, tweak the model transformations, etc. Cheers!

[gjsjohnmurray]

I'm excited to see the power of Structure101 now being harnessed for TS/JS and applied to this codebase, where architecture is held in such high regard.

[bpasero]

I don't understand the layer violation from vs/workbench/api/common to vs/workbench/contrib/testing. It is true that vs/workbench/api/* is not very strict, i.e. can depend on pretty much everything, but I am not sure what it would bring to enforce layers for API, because API span pretty much everything we provide from core+contrib?

If we wanted strict layers for our API, we would have to move the API pieces into each respective component (e.g. vs/workbench/contrib/testing/api), not even sure that would be possible or desired.

[sglebs]

@bpasero it may be the case that the existing "packages"(folders) are not true components and/or layers, so circularity among them is not really a violation of the Acyclic Dependency Principle among components. In this case a logical layer containing more than one "physical" package/folder can be created as well (at least in Structure101 this is easy), so you might have circularity inside but the thing is supposed to be reused as a whole, so no big problem. Other characteristics of the layers/components should be checked (Reuse-release Equivalence Principle, etc - more details here) to decide on the proper layering. Alternatively you can create a logical layer/component with parts from separate physical folders. For example, one can create an [api] folder/package/layer as a logical thing and enforce rules for it, despite having parts spread out "physically" across folders. The possibilities are many, but I do not know how to enforce this with eslint rules.

GOTCHA: Note that bundling packages/components to form a bigger logical package may get you off the hook regarding Tangle at higher levels, but it may produce a bigger component, which increases what Structure101 calls Fat. Here is the current situation of Fat Versus Tangle in VSCode according to the analysis I did:

Trading Tangle for Fat won't get you any closer to the origin (0,0) in most cases.

Feel free to contact me privately if you want to bounce some ideas or just want to see how things are in the tool. If I can be of any help to the project, just let me know.

[alexdima]

Thank you for the analysis!

Unfortunately I was unsuccessful in running Structure101 on our sources. On macOS, I could only find version 1.1.0 of the plugin, which after a chmod +x macos/js2s101 and some tweaks to our tsconfig.json did eventually execute, but failed with Out-Of-Memory. I switched to Windows, where I found version 1.1.3-rc-1 of the plugin, which worked out of the box, but also eventually (after about 5-10 minutes of analysis) failed with Out-Of-Memory. Are there some flags or settings that I could tweak to give it more memory? If js2s101 is using nodejs under the hood, in nodejs it is usually possible to use something like node --max_old_space_size=8192 to signal that it can consume more heap. Is there a setting that I can configure somewhere to pass that argument and overcome the Out-Of-Memory?

In parallel with this issue, prompted by https://github.com/microsoft/vscode/issues/140857, I spent some time to improve our rules and our ESLint plugin that validates our imports. I pushed those improvements via https://github.com/microsoft/vscode/commit/62b0d5cb815aa8161ffdb6550ed1787e4bffdd17 . I have also identified about 9 layer breakers, 2 of which in test files (marked in our sources with eslint-disable-next-line code-import-patterns). I will create issues for each of those and cross-reference them with the previous findings from this issue.

[alexdima]

Going through problems reported in this issue (using ✅ to mark problems that are tracked in other issues):

• ✅ platform is using editor in contextScopedHistoryWidget.ts
  • this is now flagged by our code import eslint plugin.
  • tracked in https://github.com/microsoft/vscode/issues/140856
• ✅ bootstrap-amd.js uses "vs" and vice-versa:
  • vs/editor/test/common/model/benchmark/bootstrap.js: this is a test script that can be launched via node vs/editor/test/common/model/benchmark/bootstrap.js. This will auto-generate tests for our text buffer implementation (it generates buffers and edits and execute these edits and compares with a simple non efficient text buffer implementation). I don't think I have launched this script in years and maybe we can just delete it, but it isn't a layer breaker, it is just a test utility script. Tracked in https://github.com/microsoft/vscode/issues/141005
  • src/vs/server/cli.js, src/vs/server/main.js: these are the entry points to vscode-server. I've started a discussion about them in https://github.com/microsoft/vscode/issues/140997
• ✅ base uses platform
  • ipc.net.test.ts, quickinput.test.ts
  • these are now flagged by our code import eslint plugin.
  • tracked in https://github.com/microsoft/vscode/issues/140982
  • tracked in https://github.com/microsoft/vscode/issues/140983
• ✅ regarding tests being separate from production code:
  • the Model Transformations are slightly incorrect because we have a feature called testing, so */testing/* or */*Testing.ts should not be mapped to a separate [test] namespace.
  • a layer breaker was indeed in snippetsService.ts
  • https://github.com/microsoft/vscode/issues/140986
  • this and another occurrence is now found by our code import eslint plugin:
  • https://github.com/microsoft/vscode/issues/140987
• ✅ regarding vs/platform/diagnostics <-> vs/platform/launch
  • it looks like ILaunchMainService has a method called getRemoteDiagnostics that might be better to be moved to IDiagnosticsService. I'm not sure who owns this code anymore. @bpasero do you know?
  • tracked in https://github.com/microsoft/vscode/issues/141025
• ✅regarding the much bigger tangle:
  • the tangle is very likely caused by implementations of services depending on other services. For example, we have a log service implementation which uses the file service to write to disk, or we use the log service for logging in the implementation of the environment service. Although very complex looking, I don't think this is necessarily a problem, as the service interfaces look clean. But I think there might be opportunities to split/regroup some services.
  • I'd really love to study this in more detail if I get Structure101 running locally. 👏 In code that I wrote, I could immediately spot that KeybindingResolver.contextMatchesRules should be moved from vs/platform/keybinding/ to vs/platform/contextkey/. Tracked in https://github.com/microsoft/vscode/issues/141007
  • I've also spotted vs/platform/windows depending on vs/platform/extensionManagement. I've opened https://github.com/microsoft/vscode/issues/141010
  • tracking the resulting tangle in https://github.com/microsoft/vscode/issues/141154
• ❓regarding the workbench tangle:
  • I think there is opportunity for improvement here.
  • we could aim to disallow importing workbench/api/~ in workbench/services/~. I've created https://github.com/microsoft/vscode/issues/141003 for discussion.
  • we could also consider merging workbench/services/~ into workbench/~.

@sglebs I'd love to get Structure101 running. Do you have any insights into how to overcome the Out-Of-Memory error that I'm running into?

[sglebs]

@alexdima did you point s101 at the sources in vscode/src ? Or did you point it at the root dir "vscode"? You will see that my documented steps say to point at vscode/src and the tsconfig file vscode/src/tsconfig.json.

I will investigate the Out of Memory Error, perhaps pointing it at the root folder vscode (instead of vscode/src like I did).

Thanks!

[sglebs]

@alexdima 1.1.0 does not support the tsconfig file of vscode, make sure you run 1.1.3. It should now be available there now (I think it had not been uploaded to the s101 site yet by the time you tried).

In my case I actually deleted my ~/structure101g/flavors/net.betterdeveloper.javascript.imports_1.1.0 to make sure I would not make a mistake of running it. Version 1.1.3 supports the tsconfig format present in vscode. Also, 1.1.0 was quite slow.

Try 1.1.3 pointing at vscode/src and let me know how it goes please.

[bpasero]

@alexdima

it looks like ILaunchMainService has a method called getRemoteDiagnostics that might be better to be moved to IDiagnosticsService. I'm not sure who owns this code anymore

I can look into that if you report an issue.

[sglebs]

@alexdima I adjusted Model Transformation according to your insider info that "the Model Transformations are slightly incorrect because we have a feature called testing, so /testing/ or /Testing.ts should not be mapped to a separate [test] namespace" and now I only see 2 violations there:

vs/workbench/contrib/snippets/browser/snippetsService.ts    imports [test]/vs/editor/test/common/modes/testLanguageConfigurationService.ts      
vs/workbench/contrib/testing/common/testStubs.ts    imports [test]/vs/workbench/contrib/testing/test/common/ownedTestCollection.ts      

The adjusted Model Transformation rules are:

  <transformations>
    <transformation in="*/test/*" out="[test]/{1}/test/{2}" />
    <transformation in="*/*.test.ts" out="[test]/{1}/{2}.test.ts" />
    <transformation in="*/*.integrationTest.ts" out="[test]/{1}/{2}.integrationTest.ts" />
    <transformation in="*/workbenchTestServices.ts" out="[test]/{1}/workbenchTestServices.ts" />
    <transformation in="[test]/[test]/[test]/*" out="[test]/{1}" />
    <transformation in="[test]/[test]/*" out="[test]/{1}" />
  </transformations>

(The last 2 rules are necessary for the cases where more than 1 rule is applied to the same element - a sort of clean-up at the end)

[alexdima]

did you point s101 at the sources in vscode/src ? Or did you point it at the root dir "vscode"?

🤦 That was the problem! Sorry about that, I'm now able to run the analysis. Thank you!

[sglebs]

@alexdima Glad to hear you got it going! Over here, even pointing at the root folder (incorrectly) I can't cause Out of Memory. I have a 2016 MacBook Pro, 16GB RAM here (old machine by today's standards). Do you mind sharing detailed info on the hardware that caused OoM with plugin version 1.1.3 pointing at the root folder? Is it an M1 perhaps? Or maybe an even older Mac? Thanks!

[alexdima]

@sglebs I am trying on a MacBook Pro 2019, 32GB RAM.

There must have been some unlucky timing, maybe you were releasing 1.1.3rc-1 exactly when I was trying to install it. Now, on macOS I see 1.1.3rc-1, but when I tried earlier I only saw 1.1.0, so the macOS Out-Of-Memory occurred with 1.1.0. I also pointed to vscode instead of vscode/src.

I then switched to a Windows 11 VM with 8GB, where I could see 1.1.3rc-1, but which failed to install the first few times (I think the zip was being uploaded at the same time I was trying to download/install it). Also here I pointed to vscode instead of vscode/src when the Out-Of-Memory occurred.

But after correcting and pointing to vscode/src, the analysis runs fine now for me 💯 .

[sglebs]

@alexdima regarding your observation "the tangle is very likely caused by implementations of services depending on other services. For example, we have a log service implementation which uses the file service to write to disk, or we use the log service for logging in the implementation of the environment service." it is important to note that services using services is ok if the dependency graph is acyclic (ADP - Acyclic Dependency Principle). You will only get a tangle in the tool if A uses B and B uses A (directly or indirectly). The question to ask is: should they have these cyclic dependencies? Is this desirable? Sometimes a simple service injection at runtime for the "feedback dependency" will solve it. Or maybe the feedback dependency is not supposed to be there at all. You get the idea. (In concrete terms, I mean if the Environment Service needs the Logging Service and vice-versa, just decide who the "bottom-most" one is and inject the "above" one as a runtime dependency instead of compile-time dependency, as this will make the bottom one reusable on its own due to no tangle). Just some thoughts, I am not an expert on the inner workings of the current code. Just my 2 cents. Congrats on the work!

[sglebs]

@alexdima FYI the plugin generates FAT and SIZE numbers for files. FAT comes from the total cyclomatic complexity of the file. Size comes from the lines of code (LOC). S101 also computes FAT for modules, and then the final FAT % shown in that nice green/orange 2D graph. You may want to have FAT under control as well.

@chedgers do you care to talk a bit about Fat as well? Share some wisdom applied here? Thanks!

[chedgers]

"Fat" is too much complexity in one place for a developer to easily understand. The s101 standard is to use cyclomatic complexity for functions since "number of paths" is a better measure of understandablilty (than say just LoC). The same principle is used at higher levels of composition. So at the file level, we count the number of function-to-function dependencies (rather than say just the number of functions). At the folder level it's the number of file-to-file dependencies.

Containment is a kind of coupling, so a Fat file can be dependent on/from an excessive number of other files - compared to if it was split into several simpler files, each with a fewer dependencies. In this way excessive Fat makes a codebase harder to restructure (to e.g. remove tangles, fix layering). Often splitting a Fat file is a useful precursor to resolving layering problems. The reduced granularity of dependency making it easier to relocate a subset of files/functions and remove feedback between folders... Limiting Fat as the code develops keeps the codebase more understandable and flexible.

Actually given the JS/TS flavor stops at the file level, I was pleasantly surprised to see a measure of Fat for that @sglebs - I presume that's the sum of the CC for all functions in the file?

[sglebs]

That is correct, @chedgers . The plugin generates Fat for a file as being the sum of the CC for all code in the file. It also generates the Size property as being total LOC of the file.

[bpasero]

Is there a way to configure certain layers to be the same layer? These two:

• src/vs/workbench
• src/vs/workbench/services

Are a somewhat expected tangle and it is probably unrealistic to untangle it but it makes it harder to see other tangles.

[jrieken]

That's what transformations do. It might work if you map vs/workbench/services/* onto vs/workbench (via Project > Tranformations)

[chedgers]

Yes transformations is a good way to do that. You could also use "actions" to do the same thing... right-click/unwrap on the services folder would have the same effect on the model as @jrieken's xform suggestion. Or you could delete (right-click/cut) the dependencies themselves to remove them from the model. Any such "actions" are added to the current Action List and are re-applied any time you load the model (if that Action List is selected).

[sglebs]

I am curious: was our JS/TS plugin for Structure101 useful after all? Have the tangles been mastered/eliminated? Were the ESLint rules patched to be more robust? I ended up mentioning this incident at https://www.linkedin.com/pulse/javascripttypescript-code-does-have-mess-marcio-marchini/