Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[analyzer] analyzer starts leaking when typing a switch expression #52352

Closed
incendial opened this issue May 11, 2023 · 31 comments
Closed

[analyzer] analyzer starts leaking when typing a switch expression #52352

incendial opened this issue May 11, 2023 · 31 comments
Assignees
Labels
area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. fe-analyzer-shared-parser-recovery Issues with the shared parser's handling of incorrect code P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@incendial
Copy link
Contributor

incendial commented May 11, 2023

When I type the following code, analyzer memory usage just skyrockets. Restarting the process via VS Code does not kill the existing one.

Screenshot 2023-05-11 at 17 04 33 Screenshot 2023-05-11 at 17 00 21

Been able to repro this 2 times in a row.

Here is a video on produced errors.

Screen.Recording.2023-05-11.at.17.06.20.mov

Let me know what else I need to provide for this to be fixed.

#### General info

- Dart 3.0.0-417.4.beta (beta) (Tue May 2 10:26:14 2023 +0000) on "macos_x64"
- on macos / Version 12.6 (Build 21G115)
- locale is en-GB

#### Project info

- sdk constraint: '>=3.0.0-417.1.beta <4.0.0'
- dependencies: ansicolor, collection, crypto, file, glob, html, meta, path, pub_semver, recase, source_span, xml, yaml
- dev_dependencies: lints, mocktail, test
- elided dependencies: 4

#### Process info

|  Memory |   CPU | Elapsed time | Command line                                                                           |
| ------: | ----: | -----------: | -------------------------------------------------------------------------------------- |
| 6996 MB | 67.8% |        07:31 | dart language-server --protocol=lsp --client-id=VS-Code --client-version=3.65.20230510 |
@incendial
Copy link
Contributor Author

And it just keeps growing...
Screenshot 2023-05-11 at 17 10 08

@bwilkerson bwilkerson added P1 A high priority bug; for example, a single project is unusable or has many test failures area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels May 11, 2023
@bwilkerson
Copy link
Member

@scheglov

@scheglov
Copy link
Contributor

It would be nice if you provided snippets of code with your issues.
It is somewhat inconvenient try to retype code from video :-P

@scheglov
Copy link
Contributor

I cannot reproduce it with a simple example.

  solo_test_X() async {
    await assertErrorsInCode(r'''
void f(Object? x) {
  final innerIf = switch (x) {
    Block(statements: [IfStatement inner]) => inner,
  };
}

class Block {
  List<Statement> get statements => [];
}

class Statement {}

class IfStatement extends Statement {}
''', [
      error(WarningCode.UNUSED_LOCAL_VARIABLE, 28, 7),
      error(CompileTimeErrorCode.NON_EXHAUSTIVE_SWITCH_EXPRESSION, 38, 6),
    ]);

    final node = findNode.singleSwitchExpression;
    assertResolvedNodeText(node, r'''
''');
  }

From the fact that it uses CPU, it seems that it might got into an infinite cycle somewhere.

Locally, I always run DAS with Observatory enabled.
To do so, I set dart.server.vm.options in my IntelliJ to --no_dds --observe:8181 --serve-observatory --disable-service-auth-codes, and then use cpu profile (table) on (main) isolate to see if there is anything interesting.
image

@incendial
Copy link
Contributor Author

Sure, will do!

It happened on typing (at least that how it looked). So I'm not sure that tests can spot it.

@scheglov
Copy link
Contributor

I tried to type something similar in the IDE, outside tests, but cannot get it into "sticky errors" when it cycles :-(

void f(IfStatement node) {
  final innerIf = switch (node.thenStatement) {
    Block(statements: [IfStatement inner]) => inner,
    _ => null,
  };
  print(innerIf);
}

@incendial
Copy link
Contributor Author

incendial commented May 11, 2023

Interesting, now I also cant repro it in one go. I understood that when "dart.fix" added final:

final innerIf = switch (node.thenStatement) {
  Block(statements: [final IfStatement inner]) => inner,
};

will try again, maybe it has something to do with the order in which code is added.

@incendial
Copy link
Contributor Author

Yep, got it again. Trying to extract steps.

@incendial
Copy link
Contributor Author

incendial commented May 11, 2023

Screen.Recording.2023-05-11.at.23.14.46.mov
Screen.Recording.2023-05-11.at.23.16.31.mov


From my edit history exactly before the initial report.

Screenshot 2023-05-11 at 23 18 52

Affects dcm as well.

@incendial
Copy link
Contributor Author

As you can see on the second video analyzer is already slowed down, but it was not recorded instantly, probably after 2-3 GB leaked.

@incendial
Copy link
Contributor Author

There is only one other issue present in the file (maybe it's related somehow).

Screenshot 2023-05-11 at 23 22 08
  bool _isCollapsibleIf(IfStatement node) =>
      node.elseStatement == null && node.expression.;

@incendial
Copy link
Contributor Author

incendial commented May 11, 2023

After the process is killed manually, this code does not trigger the leak (not typing this code, but its presence).

 final innerIf = switch (node.thenStatement) {
        Block(statements: [final IfStatement inner]) =>inner,
      };

@incendial
Copy link
Contributor Author

Screenshot 2023-05-11 at 23 23 52

@incendial
Copy link
Contributor Author

Let me know if you need anything else.

@scheglov
Copy link
Contributor

It might be not obvious, but I cannot reproduce it.
From the provided info I see that there is a problem, but I have no idea why and where.

@incendial
Copy link
Contributor Author

I'll try to extract the exact step, I think I'm close.

@incendial
Copy link
Contributor Author

final innerIf = switch (node.thenStatement) {
        Block(statements: [if])
      };

can you try this one? restarting analyzer here leads to immediate leak.

@incendial
Copy link
Contributor Author

incendial commented May 11, 2023

Note: I got if by typo in final. Or I wanted to write IfStatement 🤔

@scheglov
Copy link
Contributor

It seems to did it! y IDE does not update semantic highlighting, and the DAS process eats CPU.
Thank you!

@scheglov
Copy link
Contributor

It looks to be cycling in the parser.
image

@incendial
Copy link
Contributor Author

incendial commented May 11, 2023

You're welcome! This one was very tricky.
Actually, I was assuming it is something related to the parser.

@scheglov
Copy link
Contributor

I cleaned it up to

void f(Object? x) {
  switch (x) {
    case [if]
  };
}

This code cycles the unit test code above.

@scheglov
Copy link
Contributor

@stereotype441 could you take a look?

@incendial
Copy link
Contributor Author

It even does not matter whether it's case [] or ObjectPattern with []? Interesting. So it might be something old?

@stereotype441
Copy link
Member

I'm able to reproduce this and I'm investigating.

@stereotype441 stereotype441 self-assigned this May 11, 2023
@stereotype441 stereotype441 added area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. fe-analyzer-shared-parser-recovery Issues with the shared parser's handling of incorrect code labels May 11, 2023
@stereotype441 stereotype441 removed the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label May 11, 2023
@stereotype441
Copy link
Member

Thanks @incendial and @scheglov for tracking down such a small repro! It made it a lot easier to debug the problem.

I've landed a fix in the main development branch, and I've requested approval too cherry-pick it into the 3.0.1 stable release (#52376).

@incendial
Copy link
Contributor Author

Thanks! Considering the fix was in _fe_analyzer_shared, do you have a release date for the new version?

@stereotype441
Copy link
Member

The code that gets run under the hood when you're interacting with your editor is the code that's included in the Dart SDK, even if that code comes ultimately from the _fe_analyzer_shared or analyzer package instead of directly from the analysis server. So it actually doesn't matter when we release _fe_analyzer_shared--you'll get the fix when you install a version of the SDK that includes it.

So far there's no published version of the SDK that includes the fix. Hopefully it will show up in the dev channel later today (dev channel builds have been happening about once every four hours recently). After that it will hopefully show up in the 3.0.1 stable hotfix (assuming my cherry-pick request gets approved). I'm currently checking whether there's a target date for 3.0.1 that I can share publicly.

@incendial
Copy link
Contributor Author

incendial commented May 12, 2023

I work on a tool that uses analyzer and _fe_analyzer_shared directly and I'm asking to be able to pick the changes as well. The only way I can do that is when there is a new published version available in pub.

So it matters to me 🙂.

@stereotype441
Copy link
Member

Ah, ok. In that case I'll defer to @scheglov, who usually does the releases of analyzer and _fe_analyzer_shared.

@scheglov
Copy link
Contributor

I'm not ready to publish yet, would like to wait a few days after a3956e4

https://dart-review.googlesource.com/c/sdk/+/303012 for now, not that it is much useful for you, but it record that this issue is fixed :-)

copybara-service bot pushed a commit that referenced this issue May 12, 2023
There is a demand for a new analyzer version.
#52352 (comment)

I'm not ready to publish yet, lets wait for a couple of days after
landing `InvalidType`. But I'd like to record issues fixed.

Change-Id: I045ba904e3a229939fca5e37d63f16624cff8872
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/303012
Commit-Queue: Konstantin Shcheglov <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 19, 2023
…es progress.

In rare circumstances involving syntax errors, the `parsePattern`
method inserts a synthetic token but does not consume any
tokens. Usually when this happens it's not a problem, because whatever
method is calling `parsePattern` consumes some tokens, so the parser
always makes progress. However, when parsing list patterns, after
calling `parsePattern`, the parser would look for a `,`, and if it
didn't find one, it would supply a synthetic `,` and call
`parsePattern` again, resulting in an infinite loop. A similar
situation happened with map patterns, though the situation was more
complex because in between the calls to `parsePattern`, the parser
would also create synthetic key expressions and `:`s.

To fix the problem, when parsing a list or map pattern, after the call
to `parsePattern`, the parser checks whether any tokens were
consumed. If no tokens were consumed, it ignores the next token from
the input stream in order to make progress.

I also investigated whether there were similar issues with
parenthesized/record patterns and switch expressions, since those
constructs also consist of a sequence of patterns separated by tokens
and other things that could in principle be supplied
synthetically. Fortunately, parser recovery doesn't get into an
infinite loop in those cases, so I didn't make any further
changes. But I did include test cases to make sure.

Fixes: #52376

Bug: #52352
Change-Id: Ic9abf1bb82b8d7e1eb18e8a771a2ff9116fcfe21
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/302803
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/303081
Reviewed-by: Jens Johansen <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. fe-analyzer-shared-parser-recovery Issues with the shared parser's handling of incorrect code P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

4 participants