Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Several bug fixes #280

Merged
merged 2 commits into from
Oct 18, 2018
Merged

Several bug fixes #280

merged 2 commits into from
Oct 18, 2018

Conversation

AlexanderSher
Copy link
Contributor

  • Fix issue from ... import shows incorrect completions #95: from ... import shows incorrect completions. TryGetCompletionsInFromImport didn't use information from ModuleInfo._referencedModules, trying to find module directly inside AST.
  • Fix issue Reloading modules leaks memory #109: Reloading modules leaks memory. ModuleInfo.Clear didn't remove back references from ModuleReference instances
  • Fix issue Huge memory usage when analysis 9 lines python code #278: Huge memory usage when analysis 9 lines python code. Issue caused by Json.Net serializing a cycle in DocumentSymbol tree. Server.ToDocumentSymbols uses Dictionary<IMemberResult, List<IMemberResult>> to make a tree, and MemberResult's GetHashCode and Equals didn't take care of the scope. As a result, all results with equal parent name were collapsed into one level, which created a loop in a resulting tree where Json.Net has been hanging.
  • Some code clean-up (dead code paths, auto-properties, pattern matching, etc.)

- Fix issue microsoft#109: Reloading modules leaks memory
- Fix issue microsoft#278: Huge memory usage when analysis 9 lines python code
- Some code clean-up
@jakebailey
Copy link
Member

I can confirm that this fixes #278. Nice!

@@ -22,16 +22,14 @@ namespace Microsoft.PythonTools.Parsing.Ast {
public class DottedName : Node {
private readonly NameExpression[] _names;

public DottedName(NameExpression[] names) {
public DottedName(NameExpression[]/*!*/ names) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the meaning of /*!*/ introduced in a few of these files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment that I've used to mark parameter that can't be null. I can add Check calls instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to change it, I just hadn't seen that before and was curious. Grepping through the project shows its use a number of other times as well (so I just hadn't noticed).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear - this is just a comment. It isn't handled by any analyzers. I really expect to see non-nullable reference types in C# 8: https://github.com/dotnet/csharplang/wiki/Nullable-Reference-Types-Preview

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I know. I actually went searching to see if that feature existed yet when you mentioned it. 🙂

}

public void ClearReferencedModules() {
foreach (var moduleReference in _referencedModules.Values.Distinct()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if Values can be null (when module is missing) - perhaps MaybeEnumerate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_referencedModules is a dictionary, it is always initialized and it is readonly.

…-server into Fix95

# Conflicts:
#	src/Analysis/Engine/Test/CompletionTest.cs
@AlexanderSher AlexanderSher merged commit 18e25f1 into microsoft:master Oct 18, 2018
@AlexanderSher AlexanderSher deleted the Fix95 branch October 18, 2018 21:03
jakebailey pushed a commit to jakebailey/python-language-server that referenced this pull request Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants