Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1403983002:
AS strong-mode .analysis_options awareness. (Closed)
|
Created:
5 years, 2 months ago by pquitslund Modified:
5 years, 2 months ago CC:
reviews_dartlang.org, Jennifer Messerly Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAS strong-mode .analysis_options awareness.
R=brianwilkerson@google.com, leafp@google.com
Committed: https://github.com/dart-lang/sdk/commit/e5f3bb163bd9d2cd495566d421e3177b7ba6b0a5
Patch Set 1 #
Total comments: 8
Patch Set 2 : no_solo #Patch Set 3 : check-fix #
Messages
Total messages: 13 (2 generated)
pquitslund@google.com changed reviewers: + brianwilkerson@google.com, leafp@google.com
Note that we'll want to update the context_manager to re-analyze sources when this setting changes (lest we require users to re-start server). Tracking issue for that is here: https://github.com/dart-lang/sdk/issues/24574
jmesserly@google.com changed reviewers: + jmesserly@google.com
random comment https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... File pkg/analysis_server/lib/src/context_manager.dart (right): https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... pkg/analysis_server/lib/src/context_manager.dart:493: options.strongMode = true; we had an issue at some point where we also needed to set the SDK context: https://github.com/dart-lang/dev_compiler/blob/6fe74a269f7877b6aae3d03bfd9be9... Not sure if that's still the case or not... Maybe Leaf/Brian remember what the issue was?
https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/test/co... File pkg/analysis_server/test/context_manager_test.dart (right): https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/test/co... pkg/analysis_server/test/context_manager_test.dart:100: solo_test_strong_mode_analysis_option() async { remove "solo_"?
Thanks John! https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... File pkg/analysis_server/lib/src/context_manager.dart (right): https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... pkg/analysis_server/lib/src/context_manager.dart:493: options.strongMode = true; On 2015/10/13 20:00:28, John Messerly wrote: > we had an issue at some point where we also needed to set the SDK context: > https://github.com/dart-lang/dev_compiler/blob/6fe74a269f7877b6aae3d03bfd9be9... > > Not sure if that's still the case or not... Maybe Leaf/Brian remember what the > issue was? Oh! Interesting. Leaf/Brian? https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/test/co... File pkg/analysis_server/test/context_manager_test.dart (right): https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/test/co... pkg/analysis_server/test/context_manager_test.dart:100: solo_test_strong_mode_analysis_option() async { On 2015/10/13 20:01:02, John Messerly wrote: > remove "solo_"? Duh. Yeah. Thanks. Should have finished lunch! ;)
LGTM https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... File pkg/analysis_server/lib/src/context_manager.dart (right): https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... pkg/analysis_server/lib/src/context_manager.dart:489: if (strongMode != null && strongMode == true) { The expression "strongMode != null" isn't necessary. https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... pkg/analysis_server/lib/src/context_manager.dart:493: options.strongMode = true; > Maybe Leaf/Brian remember what the issue was? I don't remember. But eventually, to support this with the semantics we talked about yesterday, we'll probably need two SDK's: one with strong mode enabled and one with strong mode disabled. We should be able to create them lazily.
On 2015/10/13 20:12:02, Brian Wilkerson wrote: > LGTM > > https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... > File pkg/analysis_server/lib/src/context_manager.dart (right): > > https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... > pkg/analysis_server/lib/src/context_manager.dart:489: if (strongMode != null && > strongMode == true) { > The expression "strongMode != null" isn't necessary. Double-duh. Time for coffee! :) > > https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... > pkg/analysis_server/lib/src/context_manager.dart:493: options.strongMode = true; > > Maybe Leaf/Brian remember what the issue was? > > I don't remember. But eventually, to support this with the semantics we talked > about yesterday, we'll probably need two SDK's: one with strong mode enabled and > one with strong mode disabled. We should be able to create them lazily. I'll leave that bit in your capable hands. Happy to help. (But not without coffee.)
lgtm https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... File pkg/analysis_server/lib/src/context_manager.dart (right): https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... pkg/analysis_server/lib/src/context_manager.dart:493: options.strongMode = true; On 2015/10/13 20:12:02, Brian Wilkerson wrote: > > Maybe Leaf/Brian remember what the issue was? > > I don't remember. But eventually, to support this with the semantics we talked > about yesterday, we'll probably need two SDK's: one with strong mode enabled and > one with strong mode disabled. We should be able to create them lazily. Will we be analyzing the SDK in strong mode with this CL, or only user code? It shouldn't make much difference now, but once we start adding generic method annotations that might change.
https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... File pkg/analysis_server/lib/src/context_manager.dart (right): https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... pkg/analysis_server/lib/src/context_manager.dart:493: options.strongMode = true; On 2015/10/13 20:12:02, Brian Wilkerson wrote: > > Maybe Leaf/Brian remember what the issue was? > > I don't remember. But eventually, to support this with the semantics we talked > about yesterday, we'll probably need two SDK's: one with strong mode enabled and > one with strong mode disabled. We should be able to create them lazily. yeah. I guess this isn't a big issue yet, until we try using generic methods in the SDK. At that point, we'd also likely want to be able to point at an experimental SDK sources to analyze. I guess we can try it out and see how it's working now, and cross that other bridge when we come to it.
On 2015/10/13 20:20:54, John Messerly wrote: > https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... > File pkg/analysis_server/lib/src/context_manager.dart (right): > > https://codereview.chromium.org/1403983002/diff/1/pkg/analysis_server/lib/src... > pkg/analysis_server/lib/src/context_manager.dart:493: options.strongMode = true; > On 2015/10/13 20:12:02, Brian Wilkerson wrote: > > > Maybe Leaf/Brian remember what the issue was? > > > > I don't remember. But eventually, to support this with the semantics we talked > > about yesterday, we'll probably need two SDK's: one with strong mode enabled > and > > one with strong mode disabled. We should be able to create them lazily. > > yeah. I guess this isn't a big issue yet, until we try using generic methods in > the SDK. At that point, we'd also likely want to be able to point at an > experimental SDK sources to analyze. > > I guess we can try it out and see how it's working now, and cross that other > bridge when we come to it. Cool. I'll land this as is and we can iterate. Thanks!
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as e5f3bb163bd9d2cd495566d421e3177b7ba6b0a5 (presubmit successful).
Message was sent while issue was closed.
SGTM + LGTM :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
