From 43e83c527f5ecb0fe03e0f57ecd82b077a47a993 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Tue, 6 Feb 2024 20:21:51 +0000 Subject: [PATCH] Fix the conflict in user settings, add tests --- packages/jupyterlab-lsp/schema/plugin.json | 1 + packages/jupyterlab-lsp/src/settings.spec.ts | 249 +++++++++++++++++++ packages/jupyterlab-lsp/src/settings.ts | 197 ++++++++++++--- 3 files changed, 406 insertions(+), 41 deletions(-) diff --git a/packages/jupyterlab-lsp/schema/plugin.json b/packages/jupyterlab-lsp/schema/plugin.json index e169eabb5..d95bcce6b 100644 --- a/packages/jupyterlab-lsp/schema/plugin.json +++ b/packages/jupyterlab-lsp/schema/plugin.json @@ -32,6 +32,7 @@ "language_servers": { "title": "Language Server", "description": "Language-server specific configuration, keyed by implementation, e.g: \n\npyls: {\n serverSettings: {\n pyls: {\n plugins: {\n pydocstyle: {\n enabled: true\n },\n pyflakes: {\n enabled: false\n },\n flake8: {\n enabled: true\n }\n }\n }\n }\n}\n\nAlternatively, using dotted naming convention:\n\npyls: {\n serverSettings: {\n \"pyls.plugins.pydocstyle.enabled\": true,\n \"pyls.plugins.pyflakes.enabled\": false,\n \"pyls.plugins.flake8.enabled\": true\n }\n}", + "type": "object", "default": { "pyright": { "serverSettings": { diff --git a/packages/jupyterlab-lsp/src/settings.spec.ts b/packages/jupyterlab-lsp/src/settings.spec.ts index 4be25aafd..09e41ca43 100644 --- a/packages/jupyterlab-lsp/src/settings.spec.ts +++ b/packages/jupyterlab-lsp/src/settings.spec.ts @@ -1,8 +1,257 @@ import { SettingsSchemaManager } from './settings'; +import { ISettingRegistry } from '@jupyterlab/settingregistry'; +import { LanguageServerManager } from '@jupyterlab/lsp'; +import { JSONExt } from '@lumino/coreutils'; const DEAULT_SERVER_PRIORITY = 50; +const SCHEMA: ISettingRegistry.ISchema = { + type: 'object', + definitions: { + 'language-server': { + type: 'object', + default: {}, + properties: { + priority: { + title: 'Priority of the server', + type: 'number', + default: 50, + minimum: 1 + }, + serverSettings: { + title: 'Language Server Configurations', + type: 'object', + default: {}, + additionalProperties: true + } + } + } + }, + properties: { + language_servers: { + title: 'Language Server', + type: 'object', + default: { + pyright: { + serverSettings: { + 'python.analysis.useLibraryCodeForTypes': true + } + }, + 'bash-language-server': { + serverSettings: { + 'bashIde.enableSourceErrorDiagnostics': true + } + } + }, + patternProperties: { + '.*': { + $ref: '#/definitions/language-server' + } + }, + additionalProperties: { + $ref: '#/definitions/language-server' + } + } + } +}; + +const PYRIGHT_SCHEMA = { + $schema: 'http://json-schema.org/draft-07/schema#', + title: 'Pyright Language Server Configuration', + description: + 'Pyright Configuration Schema. Distributed under MIT License, Copyright (c) Microsoft Corporation.', + type: 'object', + properties: { + 'python.analysis.diagnosticSeverityOverrides': { + type: 'object', + description: + 'Allows a user to override the severity levels for individual diagnostics.', + scope: 'resource', + properties: { + reportGeneralTypeIssues: { + type: 'string', + description: + 'Diagnostics for general type inconsistencies, unsupported operations, argument/parameter mismatches, etc. Covers all of the basic type-checking rules not covered by other rules. Does not include syntax errors.', + default: 'error', + enum: ['none', 'information', 'warning', 'error'] + }, + reportPropertyTypeMismatch: { + type: 'string', + description: + 'Diagnostics for property whose setter and getter have mismatched types.', + default: 'none', + enum: ['none', 'information', 'warning', 'error'] + }, + reportFunctionMemberAccess: { + type: 'string', + description: 'Diagnostics for member accesses on functions.', + default: 'none', + enum: ['none', 'information', 'warning', 'error'] + }, + reportMissingImports: { + type: 'string', + description: + 'Diagnostics for imports that have no corresponding imported python file or type stub file.', + default: 'error', + enum: ['none', 'information', 'warning', 'error'] + }, + reportUnusedImport: { + type: 'string', + description: + 'Diagnostics for an imported symbol that is not referenced within that file.', + default: 'none', + enum: ['none', 'information', 'warning', 'error'] + }, + reportUnusedClass: { + type: 'string', + description: + 'Diagnostics for a class with a private name (starting with an underscore) that is not accessed.', + default: 'none', + enum: ['none', 'information', 'warning', 'error'] + } + } + }, + 'python.analysis.logLevel': { + type: 'string', + default: 'Information', + description: 'Specifies the level of logging for the Output panel', + enum: ['Error', 'Warning', 'Information', 'Trace'] + }, + 'python.analysis.useLibraryCodeForTypes': { + type: 'boolean', + default: false, + description: + 'Use library implementations to extract type information when type stub is not present.', + scope: 'resource' + }, + 'python.pythonPath': { + type: 'string', + default: 'python', + description: 'Path to Python, you can use a custom version of Python.', + scope: 'resource' + } + } +}; + +const COLLAPSED_PYRIGHT_SETTINGS = { + pyright: { + priority: 50, + serverSettings: { + 'python.analysis.autoImportCompletions': false, + 'python.analysis.extraPaths': [], + 'python.analysis.stubPath': 'typings', + 'python.pythonPath': 'python', + 'python.analysis.diagnosticSeverityOverrides.reportGeneralTypeIssues': + 'error', + 'python.analysis.diagnosticSeverityOverrides.reportPropertyTypeMismatch': + 'none', + 'python.analysis.diagnosticSeverityOverrides.reportFunctionMemberAccess': + 'none', + 'python.analysis.diagnosticSeverityOverrides.reportMissingImports': 'none' + } + } +}; + +function map(object: Object) { + return new Map(Object.entries(object)); +} + +const AVAILABLE_SESSIONS = map({ + pyright: null as any, + pylsp: null as any +}) as LanguageServerManager['sessions']; + describe('SettingsSchemaManager', () => { + describe('#expandDottedAsNeeded()', () => { + it('should uncollapse pyright defaults', () => { + const partiallyExpaneded = SettingsSchemaManager.expandDottedAsNeeded({ + dottedSettings: COLLAPSED_PYRIGHT_SETTINGS, + specs: map({ + pyright: { + display_name: 'pyright', + // server-specific defaults and allowed values + config_schema: PYRIGHT_SCHEMA + } + }) as LanguageServerManager['specs'] + }); + expect(partiallyExpaneded).toEqual({ + pyright: { + priority: 50, + serverSettings: { + 'python.analysis.autoImportCompletions': false, + 'python.analysis.diagnosticSeverityOverrides': { + reportFunctionMemberAccess: 'none', + reportGeneralTypeIssues: 'error', + reportMissingImports: 'none', + reportPropertyTypeMismatch: 'none' + }, + 'python.analysis.extraPaths': [], + 'python.analysis.stubPath': 'typings', + 'python.pythonPath': 'python' + } + } + }); + }); + }); + + describe('#transformSchemas()', () => { + it('should merge dotted defaults', () => { + const schema = JSONExt.deepCopy(SCHEMA) as any; + + // Set a few defaults as if these came from `overrides.json`: + // - using fully dotted name + schema.properties.language_servers.default.pyright.serverSettings[ + 'python.analysis.diagnosticSeverityOverrides.reportGeneralTypeIssues' + ] = 'warning'; + // - using nesting on final level (as defined in source pyright schema) + schema.properties.language_servers.default.pyright.serverSettings[ + 'python.analysis.diagnosticSeverityOverrides' + ] = { + reportPropertyTypeMismatch: 'warning' + }; + + const { defaults } = SettingsSchemaManager.transformSchemas({ + // plugin schema which includes overrides from `overrides.json` + schema, + specs: map({ + pyright: { + display_name: 'pyright', + // server-specific defaults and allowed values + config_schema: PYRIGHT_SCHEMA, + // overrides defined in specs files e.g. `jupyter_server_config.py` + workspace_configuration: { + // using fully dotted name + 'python.analysis.diagnosticSeverityOverrides.reportFunctionMemberAccess': + 'warning', + // using nesting on final level (as defined in source pyright schema) + 'python.analysis.diagnosticSeverityOverrides': { + reportUnusedImport: 'warning' + } + } + } + }) as LanguageServerManager['specs'], + sessions: AVAILABLE_SESSIONS + }); + const defaultOverrides = + defaults.pyright.serverSettings[ + 'python.analysis.diagnosticSeverityOverrides' + ]; + expect(defaultOverrides).toEqual({ + // `overrides.json`: + // - should provide `reportGeneralTypeIssues` defined with fully dotted key + reportGeneralTypeIssues: 'warning', + // - should provide `reportPropertyTypeMismatch` defined with nesting on final level + // `jupyter_server_config.py`: + reportPropertyTypeMismatch: 'warning', + // - should provide `reportFunctionMemberAccess` defined with fully dotted key + reportFunctionMemberAccess: 'warning', + // - should provide `reportUnusedImport` defined with nesting on final level + reportUnusedImport: 'warning' + // should NOT include `reportUnusedClass` default defined in server schema + }); + }); + }); + describe('#mergeByServer()', () => { it('prioritises user `priority` over the default', () => { const defaults = { diff --git a/packages/jupyterlab-lsp/src/settings.ts b/packages/jupyterlab-lsp/src/settings.ts index 67d40e44f..4d420981c 100644 --- a/packages/jupyterlab-lsp/src/settings.ts +++ b/packages/jupyterlab-lsp/src/settings.ts @@ -4,7 +4,7 @@ import { ISettingRegistry, ISchemaValidator } from '@jupyterlab/settingregistry'; -import { TranslationBundle } from '@jupyterlab/translation'; +import { TranslationBundle, nullTranslator } from '@jupyterlab/translation'; import { JSONExt, ReadonlyPartialJSONObject, @@ -21,7 +21,7 @@ import { renderCollapseConflicts } from './components/serverSettings'; import { ILSPLogConsole } from './tokens'; -import { collapseToDotted } from './utils'; +import { collapseToDotted, expandDottedPaths } from './utils'; type ValueOf = T[keyof T]; type ServerSchemaWrapper = ValueOf< @@ -76,7 +76,7 @@ function getDefaults( } /** - * Get a mutable property matching a dotted key. + * Get a mutable property matching a dotted key and a properly nested value. * * Most LSP server schema properties are flattened using dotted convention, * e.g. a key for {pylsp: {plugins: {flake8: {enabled: true}}}}` is stored @@ -86,12 +86,13 @@ function getDefaults( * properties like `reportGeneralTypeIssues` or `reportPropertyTypeMismatch`. * Only one level of nesting (on the finale level) is supported. */ -function findSchemaProperty( +function nestInSchema( properties: PartialJSONObject, - key: string -): PartialJSONObject | null { + key: string, + value: PartialJSONObject +): { property: PartialJSONObject; value: PartialJSONObject } | null { if (properties.hasOwnProperty(key)) { - return properties[key] as PartialJSONObject; + return { property: properties[key] as PartialJSONObject, value }; } const parts = key.split('.'); const prefix = parts.slice(0, -1).join('.'); @@ -103,12 +104,33 @@ function findSchemaProperty( } const parentProperties = parent.properties as PartialJSONObject; if (parentProperties.hasOwnProperty(suffix)) { - return parentProperties[suffix] as PartialJSONObject; + return { + property: parent, + value: { [suffix]: value } + }; } } return null; } +function mergePropertyDefault( + property: PartialJSONObject, + value: PartialJSONObject +) { + if ( + property.type === 'object' && + typeof property.default === 'object' && + typeof value === 'object' + ) { + property.default = { + ...property.default, + ...value + }; + } else { + property.default = value; + } +} + /** * Schema and user data that for validation */ @@ -267,6 +289,36 @@ export class SettingsSchemaManager { schema: ISettingRegistry.ISchema ) { const languageServerManager = this.options.languageServerManager; + + const { properties, defaults } = SettingsSchemaManager.transformSchemas({ + schema, + // TODO: expose `specs` upstream and use `ILanguageServerManager` instead + specs: (languageServerManager as LanguageServerManager).specs, + sessions: languageServerManager.sessions, + console: this.console, + trans: this.options.trans + }); + + schema.properties!.language_servers.properties = properties; + schema.properties!.language_servers.default = defaults; + + this._validateSchemaLater(plugin, schema).catch(this.console.warn); + this._defaults = defaults; + } + + /** + * Transform the plugin schema defaults, properties and descriptions + */ + static transformSchemas(options: { + schema: ISettingRegistry.ISchema; + specs: LanguageServerManager['specs']; + sessions: ILanguageServerManager['sessions']; + console?: ILSPLogConsole; + trans?: TranslationBundle; + }) { + const { schema, sessions, specs } = options; + const trans = options.trans ?? nullTranslator.load('jupyterlab-lsp'); + const console = options.console ?? window.console; const baseServerSchema = (schema.definitions as any)['language-server'] as { description: string; title: string; @@ -285,47 +337,41 @@ export class SettingsSchemaManager { | Record | undefined; - // TODO: expose `specs` upstream - for (let [serverKey, serverSpec] of ( - languageServerManager as LanguageServerManager - ).specs.entries()) { + for (let [serverKey, serverSpec] of specs.entries()) { if ((serverKey as string) === '') { - this.console.warn( - 'Empty server key - skipping transformation for', - serverSpec + console.warn( + `Empty server key - skipping transformation for ${serverSpec}` ); continue; } const configSchema = serverSpec.config_schema; if (!configSchema) { - this.console.warn( - 'No config schema - skipping transformation for', - serverKey + console.warn( + `No config schema - skipping transformation for ${serverKey}` ); continue; } if (!configSchema.properties) { - this.console.warn( - 'No properties in config schema - skipping transformation for', - serverKey + console.warn( + `No properties in config schema - skipping transformation for ${serverKey}` ); continue; } // let user know if server not available (installed, etc) - if (!languageServerManager.sessions.has(serverKey)) { - configSchema.description = this.options.trans.__( + if (!sessions.has(serverKey)) { + configSchema.description = trans.__( 'Settings that would be passed to `%1` server (this server was not detected as installed during startup) in `workspace/didChangeConfiguration` notification.', serverSpec.display_name ); } else { - configSchema.description = this.options.trans.__( + configSchema.description = trans.__( 'Settings to be passed to %1 in `workspace/didChangeConfiguration` notification.', serverSpec.display_name ); } - configSchema.title = this.options.trans.__('Workspace Configuration'); + configSchema.title = trans.__('Workspace Configuration'); // resolve refs for (let [key, value] of Object.entries(configSchema.properties)) { @@ -339,14 +385,14 @@ export class SettingsSchemaManager { const definitionID = value['$ref'].substring(14); const definition = configSchema.definitions[definitionID]; if (definition == null) { - this.console.warn('Definition not found'); + console.warn('Definition not found'); } for (let [defKey, defValue] of Object.entries(definition)) { configSchema.properties[key][defKey] = defValue; } delete value.$ref; } else { - this.console.warn('Unsupported $ref', value['$ref']); + console.warn('Unsupported $ref', value['$ref']); } } @@ -357,14 +403,14 @@ export class SettingsSchemaManager { for (const [key, value] of Object.entries( workspaceConfigurationDefaults )) { - const property = findSchemaProperty(configSchema.properties, key); - if (!property) { - this.console.warn( + const nested = nestInSchema(configSchema.properties, key, value); + if (!nested) { + console.warn( `"workspace_configuration" includes an override for "${key}" key which was not found in ${serverKey} schema'` ); continue; } - property.default = value; + mergePropertyDefault(nested.property, nested.value); } } // add server-specific default overrides from `overrides.json` (and pre-defined in schema) @@ -376,14 +422,18 @@ export class SettingsSchemaManager { for (const [key, value] of Object.entries( serverDefaultsOverrides.serverSettings )) { - const property = findSchemaProperty(configSchema.properties, key); - if (!property) { - this.console.warn( + const nested = nestInSchema( + configSchema.properties, + key, + value as any + ); + if (!nested) { + console.warn( `"overrides.json" includes an override for "${key}" key which was not found in ${serverKey} schema` ); continue; } - property.default = value as any; + mergePropertyDefault(nested.property, nested.value); } } @@ -399,11 +449,67 @@ export class SettingsSchemaManager { }; } - schema.properties!.language_servers.properties = knownServersConfig; - schema.properties!.language_servers.default = defaults; + return { + properties: knownServersConfig, + defaults + }; + } - this._validateSchemaLater(plugin, schema).catch(this.console.warn); - this._defaults = defaults; + /** + * Expands dotted values into nested properties when the server config schema + * indicates that this is needed. The schema is passed within the specs. + * + * This is needed because some settings, specifically pright's + * `python.analysis.diagnosticSeverityOverrides` are defined as nested. + */ + static expandDottedAsNeeded(options: { + dottedSettings: LanguageServerSettings; + specs: LanguageServerManager['specs']; + }): LanguageServerSettings { + const specs = options.specs; + const partiallyUncollapsed = JSONExt.deepCopy(options.dottedSettings); + + for (let [serverKey, serverSpec] of specs.entries()) { + const configSchema = serverSpec.config_schema; + if (!partiallyUncollapsed.hasOwnProperty(serverKey)) { + continue; + } + const settings = partiallyUncollapsed[serverKey].serverSettings; + if (!configSchema || !settings) { + continue; + } + const expanded = expandDottedPaths(settings); + + for (const [path, property] of Object.entries( + configSchema.properties + )) { + if (property.type === 'object') { + let value = expanded; + for (const part of path.split('.')) { + value = value[part] as ReadonlyJSONObject; + if (typeof value === 'undefined') { + break; + } + } + if (typeof value === 'undefined') { + continue; + } + // Add the uncollapsed value + settings[path] = value; + // Remove the collapsed values + for (const k of Object.keys(value)) { + const key = path + '.' + k; + if (!settings.hasOwnProperty(key)) { + throw Error( + 'Internal inconsistency: collapsed settings state does not match expanded object' + ); + } + delete settings[key]; + } + } + } + } + return partiallyUncollapsed; } /** @@ -434,11 +540,20 @@ export class SettingsSchemaManager { composite.language_servers ); - composite.language_servers = SettingsSchemaManager.mergeByServer( + const merged = SettingsSchemaManager.mergeByServer( collapsedDefaults.settings, collapsedUser.settings ); - this._lastUserServerSettingsDoted = composite.language_servers; + + // Uncollapse settings which need to be in the expanded form + const languageServerManager = this.options.languageServerManager; + const uncollapsed = SettingsSchemaManager.expandDottedAsNeeded({ + dottedSettings: merged, + specs: (languageServerManager as LanguageServerManager).specs + }); + + composite.language_servers = uncollapsed; + this._lastUserServerSettingsDoted = uncollapsed; if (Object.keys(collapsedUser.conflicts).length > 0) { this._warnConflicts(