Initial work on refactorings.

Initial work on refactorings.

R=brianwilkerson@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=39098

[Brian Wilkerson, 2014-08-11 16:27:48]

Some of my comments might not be appropriate, depending on how far along some of
this initial work was intended to be.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
File pkg/analysis_services/lib/correction/status.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:22: RefactoringStatus
createErrorStatus(String msg,
I would have made these named constructors:

  RefactoringStatus.error(String message, [RefactoringContext context])

It would be shorter to type, make it easier to keep these methods with the
class, and wouldn't add to the top-level name space.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:56: RefactoringStatusSeverity
_severity = RefactoringStatusSeverity.OK;
Please document this field.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:73: if (result.severity.ordinal
< entry.severity.ordinal) {
Given that _severity is the maximum severity, couldn't we return as soon as we
find the first entry with that severity?

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:154: new
RefactoringStatusEntry(severity, entry.message, entry.context));
Could we share entries when the severity has not changed? It might not be worth
it, but it looks like entries are immutable.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:166: * current severityand the
parameter's severity.
"severityand" --> "severity and"

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:170: void
merge(RefactoringStatus other) {
When I saw this method being used I had to come back and check the semantics. My
expectation from the name at the call site was that the method would return a
new "merged" result but wouldn't modify the original (and I was about to comment
that you forgot to capture the result). Turns out it's the name that's
confusing. How about "addFromStatus" or "addAll" or "addEntries"?

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:206: static
RefactoringStatusSeverity _max(RefactoringStatusSeverity a,
This should be defined on RefactoringStatusSeverity.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:353: class
RefactoringStatusSeverity {
Should this be a subclass of Enum?

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:357: static const FATAL = const
RefactoringStatusSeverity('FATAL', 4);
It would be good to document these severities, especially the difference between
FATAL and ERROR.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
File pkg/analysis_services/lib/refactoring/progress_monitor.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
pkg/analysis_services/lib/refactoring/progress_monitor.dart:11: class
NullProgressMonitor implements ProgressMonitor {
I don't understand the value of a progress monitor in the context of Server. Are
you thinking that we'll send status notifications to the client?

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
File pkg/analysis_services/lib/refactoring/refactoring.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
pkg/analysis_services/lib/refactoring/refactoring.dart:19: RenameRefactoring
createRenameRefactoring(SearchEngine searchEngine,
I don't see the value in a top-level function that takes the same arguments as
the constructor.

If you just want to insulate clients from invoking the constructor directly,
then a factory constructor on Refactoring would be more appropriate.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
pkg/analysis_services/lib/refactoring/refactoring.dart:81: * Validates
[newName].
The more information in comments the better. Perhaps something like

Validates that the [newName] is a valid identifier and does not conflict with
any existing names in any of the scopes containing the current name.

(or whatever is appropriate). Also, do we need to pass in the new name, or is it
(or can it be) expected to have been set by newName?

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
File pkg/analysis_services/lib/src/refactoring/rename.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:68: bool
isInTheSameLibrary(Element element, AnalysisContext context, Source source)
The comment and the name don't appear to correspond. And "in the same library"
as what?

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:84: bool
isPublicOrInTheSameLibrary(Element element, AnalysisContext context,
This has the same comment as the previous function but clearly does something
different. The comments should make it clear what the difference is.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:134: String get
currentName => element.displayName;
This seems strange. Should this be "oldName" so that imports are handled
correctly?

Is there a difference between "oldName" and "currentName"?

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:147: void
addEdit(SourceChange sourceChange, String description, Edit edit) {
I don't understand the value of this method. Why is it better to write

  addEdit(sourceChange, description, edit)

rather than

  sourceChange.addEdit(edit, description)

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:155: Edit edit =
createReferenceEdit(reference, newName);
As far as I can see, this is the only use of "createReferenceEdit". Seems like
it would be better to in-line it.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:168:
result.addFatalError("Choose another name.");
Is this user visible text? If so, perhaps something that explains why, such as
"The new name must be different than the current name."

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:193: List<Element>
elements = [];
This field isn't currently used (at least not in this class). Should it be
removed?

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
File pkg/analysis_services/lib/src/refactoring/rename_local.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename_local.dart:54: bool
requiresPreview() {
Do we have this in the API? If not, is there any point in having it?

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/test/c...
File pkg/analysis_services/test/correction/status_test.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/test/c...
pkg/analysis_services/test/correction/status_test.dart:161:
refactoringStatus.toString(),
I'm always hesitant to test things like toString. I don't think of toString as
part of the public API and don't believe that we should be constrained in terms
of the content of the returned string. (And it seems pointless to test
characteristics that are allowed to change.)

[scheglov, 2014-08-11 18:06:26]

PTAL

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
File pkg/analysis_services/lib/correction/status.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:22: RefactoringStatus
createErrorStatus(String msg,
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> I would have made these named constructors:
> 
>   RefactoringStatus.error(String message, [RefactoringContext context])
> 
> It would be shorter to type, make it easier to keep these methods with the
> class, and wouldn't add to the top-level name space.

Thank you!
Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:56: RefactoringStatusSeverity
_severity = RefactoringStatusSeverity.OK;
On 2014/08/11 16:27:46, Brian Wilkerson wrote:
> Please document this field.

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:73: if (result.severity.ordinal
< entry.severity.ordinal) {
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> Given that _severity is the maximum severity, couldn't we return as soon as we
> find the first entry with that severity?

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:154: new
RefactoringStatusEntry(severity, entry.message, entry.context));
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> Could we share entries when the severity has not changed? It might not be
worth
> it, but it looks like entries are immutable.

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:166: * current severityand the
parameter's severity.
On 2014/08/11 16:27:46, Brian Wilkerson wrote:
> "severityand" --> "severity and"

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:170: void
merge(RefactoringStatus other) {
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> When I saw this method being used I had to come back and check the semantics.
My
> expectation from the name at the call site was that the method would return a
> new "merged" result but wouldn't modify the original (and I was about to
comment
> that you forgot to capture the result). Turns out it's the name that's
> confusing. How about "addFromStatus" or "addAll" or "addEntries"?

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:206: static
RefactoringStatusSeverity _max(RefactoringStatusSeverity a,
On 2014/08/11 16:27:46, Brian Wilkerson wrote:
> This should be defined on RefactoringStatusSeverity.

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:353: class
RefactoringStatusSeverity {
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> Should this be a subclass of Enum?

I try to avoid using Java artefacts.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/co...
pkg/analysis_services/lib/correction/status.dart:357: static const FATAL = const
RefactoringStatusSeverity('FATAL', 4);
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> It would be good to document these severities, especially the difference
between
> FATAL and ERROR.

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
File pkg/analysis_services/lib/refactoring/progress_monitor.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
pkg/analysis_services/lib/refactoring/progress_monitor.dart:11: class
NullProgressMonitor implements ProgressMonitor {
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> I don't understand the value of a progress monitor in the context of Server.
Are
> you thinking that we'll send status notifications to the client?

Good point.
We don't need it / not useful in Dart.
Removing.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
File pkg/analysis_services/lib/refactoring/refactoring.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
pkg/analysis_services/lib/refactoring/refactoring.dart:19: RenameRefactoring
createRenameRefactoring(SearchEngine searchEngine,
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> I don't see the value in a top-level function that takes the same arguments as
> the constructor.
> 
> If you just want to insulate clients from invoking the constructor directly,
> then a factory constructor on Refactoring would be more appropriate.

Done.
However actually I missed some code.
This function is a factory, right, it should check the "element" type and return
an appropriate RenameRefactoring subtype.
Or null.
I don't know for sure if it is OK in Dart style to return null from a factory
constructor.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
pkg/analysis_services/lib/refactoring/refactoring.dart:81: * Validates
[newName].
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> The more information in comments the better. Perhaps something like
> 
> Validates that the [newName] is a valid identifier and does not conflict with
> any existing names in any of the scopes containing the current name.
> 
> (or whatever is appropriate). Also, do we need to pass in the new name, or is
it
> (or can it be) expected to have been set by newName?

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
File pkg/analysis_services/lib/src/refactoring/rename.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:68: bool
isInTheSameLibrary(Element element, AnalysisContext context, Source source)
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> The comment and the name don't appear to correspond. And "in the same library"
> as what?

I've updated the comment.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:84: bool
isPublicOrInTheSameLibrary(Element element, AnalysisContext context,
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> This has the same comment as the previous function but clearly does something
> different. The comments should make it clear what the difference is.

I've updated the comment.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:134: String get
currentName => element.displayName;
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> This seems strange. Should this be "oldName" so that imports are handled
> correctly?
> 
> Is there a difference between "oldName" and "currentName"?

The "currentName" field is gone now, "oldName" is used.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:147: void
addEdit(SourceChange sourceChange, String description, Edit edit) {
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> I don't understand the value of this method. Why is it better to write
> 
>   addEdit(sourceChange, description, edit)
> 
> rather than
> 
>   sourceChange.addEdit(edit, description)

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:155: Edit edit =
createReferenceEdit(reference, newName);
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> As far as I can see, this is the only use of "createReferenceEdit". Seems like
> it would be better to in-line it.

It will be used by other refactoring implementations.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:168:
result.addFatalError("Choose another name.");
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> Is this user visible text? If so, perhaps something that explains why, such as
> "The new name must be different than the current name."

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename.dart:193: List<Element>
elements = [];
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> This field isn't currently used (at least not in this class). Should it be
> removed?

Done.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
File pkg/analysis_services/lib/src/refactoring/rename_local.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/refactoring/rename_local.dart:54: bool
requiresPreview() {
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> Do we have this in the API? If not, is there any point in having it?

No, we don't.
But we probably should.
It is useful if we rename a class member and there are potential matches.

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/test/c...
File pkg/analysis_services/test/correction/status_test.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/test/c...
pkg/analysis_services/test/correction/status_test.dart:161:
refactoringStatus.toString(),
On 2014/08/11 16:27:47, Brian Wilkerson wrote:
> I'm always hesitant to test things like toString. I don't think of toString as
> part of the public API and don't believe that we should be constrained in
terms
> of the content of the returned string. (And it seems pointless to test
> characteristics that are allowed to change.)

Acknowledged.

[Brian Wilkerson, 2014-08-11 18:45:30]

LGTM

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
File pkg/analysis_services/lib/refactoring/refactoring.dart (right):

https://codereview.chromium.org/458063002/diff/1/pkg/analysis_services/lib/re...
pkg/analysis_services/lib/refactoring/refactoring.dart:19: RenameRefactoring
createRenameRefactoring(SearchEngine searchEngine,
> I don't know for sure if it is OK in Dart style to return null from a factory
> constructor.

I generally wouldn't, but in this case (given that it's well documented) I'm OK
with it. The other option would be to implement an InvalidRenameRefactoring that
returns a FATAL error for everything indicating that we don't know how to rename
an element of that type. Your call.

https://codereview.chromium.org/458063002/diff/20001/pkg/analysis_services/li...
File pkg/analysis_services/lib/correction/status.dart (right):

https://codereview.chromium.org/458063002/diff/20001/pkg/analysis_services/li...
pkg/analysis_services/lib/correction/status.dart:70: * Returns the
[RefactoringStatusEntry] with the highest severity, or `null`
"the [" --> "the first ["? (There could be more than one. The alternative, if
order isn't significant, would be "Return a [" and then add "If there is more
than one entry with the highest severity then there is no guarantee as to which
will be returned."

[scheglov, 2014-08-11 18:58:21]

https://codereview.chromium.org/458063002/diff/20001/pkg/analysis_services/li...
File pkg/analysis_services/lib/correction/status.dart (right):

https://codereview.chromium.org/458063002/diff/20001/pkg/analysis_services/li...
pkg/analysis_services/lib/correction/status.dart:70: * Returns the
[RefactoringStatusEntry] with the highest severity, or `null`
On 2014/08/11 18:45:29, Brian Wilkerson wrote:
> "the [" --> "the first ["? (There could be more than one. The alternative, if
> order isn't significant, would be "Return a [" and then add "If there is more
> than one entry with the highest severity then there is no guarantee as to
which
> will be returned."

Done.

[scheglov, 2014-08-11 18:58:35]

Committed patchset #2 manually as 39098 (presubmit successful).