Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439343002:
Initial implementation of AnalysisDriver. (Closed)
|
Created:
4 years, 2 months ago by scheglov Modified:
4 years, 1 month ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionInitial implementation of AnalysisDriver.
I will need to add more documentation, implement state transitions,
actually implement computing analysis results, add optimizations, etc.
TBR
R=brianwilkerson@google.com, paulberry@google.com
BUG=
Committed: https://github.com/dart-lang/sdk/commit/e7b596ce5b375642d789e97b1917fa0ffb9cffdb
Patch Set 1 #
Total comments: 32
Messages
Total messages: 7 (1 generated)
Description was changed from ========== Initial implementation of AnalysisDriver. I will need to add more documentation, implement state transitions, actually implement computing analysis results, add optimizations, etc. TBR R=brianwilkerson@google.com, paulberry@google.com BUG= ========== to ========== Initial implementation of AnalysisDriver. I will need to add more documentation, implement state transitions, actually implement computing analysis results, add optimizations, etc. TBR R=brianwilkerson@google.com, paulberry@google.com BUG= Committed: https://github.com/dart-lang/sdk/commit/e7b596ce5b375642d789e97b1917fa0ffb9cffdb ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as e7b596ce5b375642d789e97b1917fa0ffb9cffdb (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:258: _requestedFiles[path] = completer; There's a bug here. If the client calls getResult() twice for the same path without waiting for the first call to complete, the old completer will be discarded and never signaled. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:259: return completer.future; I'm surprised not to see a call to `_hasWorkStreamController.add(...)` or `_filesToAnalyze.add(path)` here. Don't we need to do something to trigger the file to be analyzed? https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:312: // Caused by Bad state: Unmatched TypeParameterElementImpl T This exception is usually caused by a bug in type inference, where it creates a type pointing to a type parameter that is not in scope. If you want to send me repro instructions I'd be happy to investigate. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:359: * Return the content in which the library represented by the given s/content/context/ https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:374: return; Why? Is this because we always get "dart:" stuff from the SDK summary? If so it would be good to have a comment documenting this. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:405: _LibraryNode libraryNode = nodes[libraryFile.uri.toString()]; Nit: how about if we change createLibraryNodes() so that it returns the library node corresponding to its argument? Then we can just do: _LibraryNode libraryNode = _logger.runTimed2(() => createLibraryNodes(libraryFile), ...); https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:627: class LibraryContext { Please add doc comments to this class and its public members (or make it private). Same with PerformanceLog and ReferencedURIs. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:653: /*=T*/ runTimed/*<T>*/(String msg, /*=T*/ f()) { I'm not comfortable with the duplication between run() and runTimed(). It's not clear when callers should use one vs. the other. AFAICT, run() has a strict superset of the functionality of runTimed(), so I would just drop this method and have all callers use run(). https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:665: runTimed2(f(), String getMsg()) { Similar concern here. I would recommend getting rid of runTimed2(), and changing callers that look like this: logger.runTimed2(() { ...computation... }, () => ...computed message...); To look like this instead: logger.run("generic message"), () { ...computation... logger.writeln(...computed message...); }); It will make the log slightly less pretty, IMHO it's more important to have a clean and simple logging API. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:737: * The client cannot remember values of this property, because its value Change "cannot" to either "should not" or "cannot safely". ("cannot" implies that it's impossible; what we're trying to communicate is that it's a bad idea) https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:753: * Current this unit is resolved, it is used to compute unlinked summaries I had trouble understanding this paragraph. Do you perhaps mean something like this? "Currently method returns an *unresolved* unit, since it is only used to compute unlinked summaries and URIs. (Performing resolution and computing errors is done in a separate analysis context). But this might change in the future." https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:790: * If the [_content] field if it is still `null`, get the content from the Drop the words "if it". https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:802: _content = ''; Do we need to record the fact that we couldn't read the file contents somewhere (in order to ensure that the user will see an appropriate error message when trying to import a non-existing file)? https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:874: bool get isReady => linked != null; It's not obvious to me why "ready" means "linked". Maybe rename this method to "isLinked"? https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:876: String get linkedHash { The name "linkedHash" sounds like it means a hash of the data in "linked", and therefore "linkedHash" should always be called after linking. But it's actually the opposite situation--we call "linkedHash" prior to linking in order to figure out whether we already have the linked data in the cache somewhere. How about renaming this to something like "dependencySignature"?
Message was sent while issue was closed.
Fixed in https://codereview.chromium.org/2450483002 https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:258: _requestedFiles[path] = completer; On 2016/10/24 11:46:21, Paul Berry wrote: > There's a bug here. If the client calls getResult() twice for the same path > without waiting for the first call to complete, the old completer will be > discarded and never signaled. Fixed. We need a list of completers here. Every of the will complete with the same value at the time when we manage to compute the result. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:259: return completer.future; On 2016/10/24 11:46:21, Paul Berry wrote: > I'm surprised not to see a call to `_hasWorkStreamController.add(...)` or > `_filesToAnalyze.add(path)` here. Don't we need to do something to trigger the > file to be analyzed? Done. Yes, we need to add a value to the "has work" stream. We don't need to add to `_filesToAnalyze` because `_requestedFiles` are handled before we look into `_filesToAnalyze`. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:312: // Caused by Bad state: Unmatched TypeParameterElementImpl T On 2016/10/24 11:46:21, Paul Berry wrote: > This exception is usually caused by a bug in type inference, where it creates a > type pointing to a type parameter that is not in scope. If you want to send me > repro instructions I'd be happy to investigate. I will try to come up with a repro today. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:359: * Return the content in which the library represented by the given On 2016/10/24 11:46:21, Paul Berry wrote: > s/content/context/ Done. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:374: return; On 2016/10/24 11:46:21, Paul Berry wrote: > Why? Is this because we always get "dart:" stuff from the SDK summary? If so > it would be good to have a comment documenting this. Done. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:405: _LibraryNode libraryNode = nodes[libraryFile.uri.toString()]; On 2016/10/24 11:46:21, Paul Berry wrote: > Nit: how about if we change createLibraryNodes() so that it returns the library > node corresponding to its argument? Then we can just do: > > _LibraryNode libraryNode = _logger.runTimed2(() => > createLibraryNodes(libraryFile), ...); Done. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:627: class LibraryContext { On 2016/10/24 11:46:21, Paul Berry wrote: > Please add doc comments to this class and its public members (or make it > private). > > Same with PerformanceLog and ReferencedURIs. Done. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:653: /*=T*/ runTimed/*<T>*/(String msg, /*=T*/ f()) { On 2016/10/24 11:46:21, Paul Berry wrote: > I'm not comfortable with the duplication between run() and runTimed(). It's not > clear when callers should use one vs. the other. > > AFAICT, run() has a strict superset of the functionality of runTimed(), so I > would just drop this method and have all callers use run(). Acknowledged. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:665: runTimed2(f(), String getMsg()) { On 2016/10/24 11:46:21, Paul Berry wrote: > Similar concern here. I would recommend getting rid of runTimed2(), and > changing callers that look like this: > > logger.runTimed2(() { > ...computation... > }, () => ...computed message...); > > To look like this instead: > > logger.run("generic message"), () { > ...computation... > logger.writeln(...computed message...); > }); > > It will make the log slightly less pretty, IMHO it's more important to have a > clean and simple logging API. Acknowledged. I completely agree. I changed the log to have only run(). https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:737: * The client cannot remember values of this property, because its value On 2016/10/24 11:46:21, Paul Berry wrote: > Change "cannot" to either "should not" or "cannot safely". > > ("cannot" implies that it's impossible; what we're trying to communicate is that > it's a bad idea) Done. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:753: * Current this unit is resolved, it is used to compute unlinked summaries On 2016/10/24 11:46:21, Paul Berry wrote: > I had trouble understanding this paragraph. Do you perhaps mean something like > this? > > "Currently method returns an *unresolved* unit, since it is only used to compute > unlinked summaries and URIs. (Performing resolution and computing errors is > done in a separate analysis context). But this might change in the future." I changed the documentation comments for this getter and _File. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:790: * If the [_content] field if it is still `null`, get the content from the On 2016/10/24 11:46:21, Paul Berry wrote: > Drop the words "if it". Done. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:802: _content = ''; On 2016/10/24 11:46:21, Paul Berry wrote: > Do we need to record the fact that we couldn't read the file contents somewhere > (in order to ensure that the user will see an appropriate error message when > trying to import a non-existing file)? I don't know yet. There is a bug with not existing source. I will add TODO and fix it soon. Then I suspect that we will get error messages automatically during DartParseTask in the context. OTOH, if/when we decide to push parsed unresolved units into analysis context, we might lose this feature. Something to cover with a test to prevent a regression. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:874: bool get isReady => linked != null; On 2016/10/24 11:46:21, Paul Berry wrote: > It's not obvious to me why "ready" means "linked". Maybe rename this method to > "isLinked"? I removed this and couple other fields later. https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:876: String get linkedHash { On 2016/10/24 11:46:21, Paul Berry wrote: > The name "linkedHash" sounds like it means a hash of the data in "linked", and > therefore "linkedHash" should always be called after linking. > > But it's actually the opposite situation--we call "linkedHash" prior to linking > in order to figure out whether we already have the linked data in the cache > somewhere. > > How about renaming this to something like "dependencySignature"? Done.
Message was sent while issue was closed.
https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:312: // Caused by Bad state: Unmatched TypeParameterElementImpl T On 2016/10/24 17:23:47, scheglov wrote: > On 2016/10/24 11:46:21, Paul Berry wrote: > > This exception is usually caused by a bug in type inference, where it creates > a > > type pointing to a type parameter that is not in scope. If you want to send > me > > repro instructions I'd be happy to investigate. > > I will try to come up with a repro today. Hm... Cannot reproduce now. I will remove try/catch and see if it happens again.
Message was sent while issue was closed.
This needs tests! https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2439343002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:703: * The [Source] this [_File] instance represent. nit: "represent" --> "represents" |
||||||||||||||||||||||||||||||||||||||||||||||
