Skip to content

Conversation

@artem-tikhomirov
Copy link
Contributor

@artem-tikhomirov artem-tikhomirov commented Jun 5, 2025

Address uses of classloading through ReloadableModule. Now, we keep classloading aspect of a module independent of SModule class hierarchy (and independent of any repository read/write access, too).

Uses of ReloadableModule.getOwnClass() have been refactored with ModuleRuntime.getOwnClass(). ClassLoaderManager listeners (low-level, module/repository-kind of notification), have been replaced with ModuleDeploymentListener of LanguageRegistry.

@artem-tikhomirov artem-tikhomirov marked this pull request as draft June 5, 2025 14:23
@artem-tikhomirov artem-tikhomirov force-pushed the artem/252/reloadable-module branch from f09aa6d to 05d01cd Compare June 9, 2025 13:26
@artem-tikhomirov artem-tikhomirov marked this pull request as ready for review June 10, 2025 09:24
@artem-tikhomirov
Copy link
Contributor Author

@wsafonov, @sergej-koscejev the change is quite straightforwad and pretty much the same in all cases, but it touches various subsystems and I don't know what would be the right approach to review it. Split, one of you, or rather subsystem author?

@sergej-koscejev sergej-koscejev self-assigned this Jun 10, 2025
@sergej-koscejev
Copy link
Collaborator

I don't think we want to be defensive about loaded classes not being assignable to expected classes but rather fail with an error or an exception in this case.

E.g.

Class<?> descriptorClass = module.getOwnClass(className); 
if (descriptorClass == null || !ISelectionIntentionsDescriptor.class.isAssignableFrom(descriptorClass)) { return; }
...

should be just

Class<?> descriptorClass = module.getOwnClass(className); 
if (descriptorClass == null) { return; }

@sergej-koscejev sergej-koscejev mentioned this pull request Jun 28, 2025
@sergej-koscejev
Copy link
Collaborator

@artem-tikhomirov I'm going to force-update this PR myself.

@sergej-koscejev sergej-koscejev force-pushed the artem/252/reloadable-module branch from 67b6fd2 to 5cecc26 Compare July 14, 2025 08:58
@sergej-koscejev
Copy link
Collaborator

@artem-tikhomirov Is the result of ModuleRuntime#getOwnClass() always non-null? It says it throws CNFE, I assume it will be thrown if the class cannot be found? Could the method be annotated with @NotNull in MPS then?

@sergej-koscejev
Copy link
Collaborator

Changes to SelectionIntentionManager submitted as #1647.

@alexanderpann
Copy link
Collaborator

@sergej-koscejev can this PR be closed or merged?

@sergej-koscejev
Copy link
Collaborator

I’m working on adjusting individual commits and submitting them as separate PRs, please leave this open for now.

CLM+ReloadableModule is way too low-level runtime management mechanism,
 which drags repository access. Use modern ModuleRuntime instead

DeployListener in AppPlugin updated to remove code that checked for the current
module being unloaded. This code would never be invoked.
Besides, no need for unload hack to see if the module has been unloaded.
Now, MPS PluginManager is client of LanguageRegistry, like the code in ProjectViewManager, and
 load/unload notifications would get in proper order together with plugin app part events.
Before, mixing CLM code (notifications immediately on module injection into repo) with PluginManager code
 (notified with delay from LanguageRegistry) indeed lead to an improper class dispose ordering.
@sergej-koscejev sergej-koscejev force-pushed the artem/252/reloadable-module branch from 26646c7 to 7cca17d Compare January 16, 2026 16:28
@sergej-koscejev sergej-koscejev merged commit 2436d0f into master Jan 16, 2026
2 checks passed
@sergej-koscejev sergej-koscejev deleted the artem/252/reloadable-module branch January 16, 2026 16:44
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.

4 participants