Add "Default" LS setting, which picks between Jedi/Pylance by jakebailey · Pull Request #16139 · microsoft/vscode-python

Jake Bailey (jakebailey) · 2021-05-04T20:31:25Z

Adds a new "Default" setting, which implies Pylance when Pylance installed, otherwise uses jedi (plus/minus the LSP experiment).

For https://github.com/microsoft/vscode-python-internalbacklog/issues/176.

Jake Bailey (jakebailey) · 2021-05-04T20:31:41Z

                        "None"
                    ],
-                    "default": "Jedi",
+                    "default": "Default",


Should I remove this change now?

Seems fine for now, why do you think this needs to be removed?

This one can probably stay regardless, but the other code makes Pylance the default (and IDK when we want to actually flip that switch; this PR or another.

I think we'll be fine; hopefully #16069 will be merged before this one, we can then merge default-language-server in main (PR here), and then when this PR makes it in default-language-server we can hold off on merging it in main again until we're good to go.

Jake Bailey (jakebailey) · 2021-05-04T20:32:02Z

                "python.languageServer": {
                    "type": "string",
                    "enum": [
+                        "Default",


I think this block could use an enumDescriptions to define the semantics of each of these.

Here you go: #16141

Jake Bailey (jakebailey) · 2021-05-04T20:33:52Z

        private readonly interpreterPathService?: IInterpreterPathService,
        private readonly interpreterSecurityService?: IInterpreterSecurityService,
-        private readonly defaultJedi?: IDefaultLanguageServer,
+        private readonly defaultLS?: IDefaultLanguageServer,


The meaning of IDefaultLanguageServer has changed to return what "Default" will mean; before it also checked the user's settings, which was too much. This file is what actually determines the final result.

Jake Bailey (jakebailey) · 2021-05-04T20:34:25Z

+            userLS === 'Default' ||
+            !Object.values(LanguageServerType).includes(userLS as LanguageServerType)
+        ) {
+            ls = this.defaultLS?.defaultLSType ?? LanguageServerType.Jedi;


defaultLS can be undefined, so I had to hardcode some default here, but I'm honestly not sure under which conditions this can be the case.

Since you removed the condition in setDefaultLanguageServer I don't see why it would ever be undefined either, unless this instance gets added before setDefaultLanguageServer is called?

I tried making it required, and hit a bunch of places that simply don't have access to the data needed, so IDK what's going on there.

Jake Bailey (jakebailey) · 2021-05-04T20:36:27Z

+    experimentService: IExperimentService,
+    extensions: IExtensions,
+): Promise<PotentialDefault> {
+    if (extensions.getExtension<ILSExtensionApi>(PYLANCE_EXTENSION_ID)) {


I would have liked to call activate here and figure out of Pylance is functional or not, but Pylance hard deps on Python, and this means that activate will block forever due to the cycle.

We'll have to rely on later checks to fallback to Jedi.

Kim-Adeline Miguel (kimadeline) · 2021-05-04T21:22:34Z

                        "None"
                    ],
-                    "default": "Jedi",
+                    "default": "Default",


Seems fine for now, why do you think this needs to be removed?

Kim-Adeline Miguel (kimadeline) · 2021-05-04T22:44:12Z

Added Karthik Nadig (@karthiknadig) because he did the JediLSP work

…new-default-ls-setting

Jake Bailey (jakebailey) · 2021-05-04T23:48:20Z

We'll probably want to change

vscode-python/src/client/activation/activationService.ts

Lines 236 to 241 in fb6b722

    
           if (serverType === LanguageServerType.JediLSP && interpreter && interpreter.version) { 
        
               if (interpreter.version.major < 3 || (interpreter.version.major === 3 && interpreter.version.minor < 6)) { 
        
                   sendTelemetryEvent(EventName.JEDI_FALLBACK); 
        
                   serverType = LanguageServerType.Jedi; 
        
               } 
        
           }

to also do the check for Pylance, since we don't do python 2 either... But I'll have to check the implications of this because I hacked the refcounting system to only have one instance of pylance at any given time.

Is Python 2 going away anytime soon? :)

Jake Bailey (jakebailey) · 2021-05-05T00:48:20Z

Fixed the title; no idea why it did that.

Kim-Adeline Miguel (kimadeline) · 2021-05-05T18:37:46Z

What happened

Jake Bailey (jakebailey) · 2021-05-05T18:46:29Z

GH really did not like that. I'll have to rebase and resend.

Kartik Raj (karrtikr)

LGTM otherwise.

Kartik Raj (karrtikr) · 2021-05-05T18:48:02Z

+        if (
+            !userLS ||
+            userLS === 'Default' ||
+            !Object.values(LanguageServerType).includes(userLS as LanguageServerType)


Not sure what this condition signifies. Does this mean if user has selected an invalid LS value, we select the default LS in those cases?

Correct. We can't trust user values from enums, so this checks for invalid ones and then selects the default if they are present. That's sort of the problem with the VS code settings API; it's too easy to make mistakes.

Kartik Raj (karrtikr) · 2021-05-05T18:52:13Z

+import { ILSExtensionApi } from '../node/languageServerFolderService';
+import { LanguageServerType } from '../types';
+
+export type PotentialDefault = LanguageServerType.Jedi | LanguageServerType.JediLSP | LanguageServerType.Node;


Need not be exported.

Good catch; this should be with IDefaultLanguageServer.

Jake Bailey (jakebailey) added 3 commits May 4, 2021 13:21

Add Default LS setting

02c5127

Remove unused code

45b6420

Remove debug log

b4c90b4

Jake Bailey (jakebailey) requested a review from Kim-Adeline Miguel (kimadeline) May 4, 2021 20:31

Jake Bailey (jakebailey) commented May 4, 2021

View reviewed changes

github-actions Bot assigned Kim-Adeline Miguel (kimadeline) May 4, 2021

github-actions Bot requested a review from Kartik Raj (karrtikr) May 4, 2021 21:13

Jake Bailey (jakebailey) changed the title ~~Bundle Pylance as part of an extension pack (not a hard dependency) (#16077)~~ Bundle Pylance as part of an extension pack (not a hard dependency) May 4, 2021

Kim-Adeline Miguel (kimadeline) reviewed May 4, 2021

View reviewed changes

Kim-Adeline Miguel (kimadeline) added skip package*.json package.json and package-lock.json don't both need updating no-changelog No news entry required labels May 4, 2021

Restore tryGet

c3d461d

Kim-Adeline Miguel (kimadeline) requested a review from Karthik Nadig (karthiknadig) May 4, 2021 22:43

Merge remote-tracking branch 'upstream/default-language-server' into …

8cbfde9

…new-default-ls-setting

Jake Bailey (jakebailey) changed the title ~~Bundle Pylance as part of an extension pack (not a hard dependency)~~ Add "Default" LS setting, which picks between Jedi/Pylance May 5, 2021

Kim-Adeline Miguel (kimadeline) closed this May 5, 2021

Kim-Adeline Miguel (kimadeline) deleted the branch microsoft:default-language-server May 5, 2021 18:28

Jake Bailey (jakebailey) mentioned this pull request May 5, 2021

Add "Default" LS setting, which picks between Jedi/Pylance #16155

Merged

Kartik Raj (karrtikr) approved these changes May 5, 2021

View reviewed changes

Uh oh!

Conversation

Jake Bailey (jakebailey) commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kim-Adeline Miguel (kimadeline) May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kim-Adeline Miguel (kimadeline) commented May 4, 2021

Uh oh!

Jake Bailey (jakebailey) commented May 4, 2021

Uh oh!

Jake Bailey (jakebailey) commented May 5, 2021

Uh oh!

Kim-Adeline Miguel (kimadeline) commented May 5, 2021

Uh oh!

Jake Bailey (jakebailey) commented May 5, 2021

Uh oh!

Kartik Raj (karrtikr) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Jake Bailey (jakebailey) commented May 4, 2021 •

edited

Loading

Kim-Adeline Miguel (kimadeline) May 4, 2021 •

edited

Loading