Add utilities for creating SourceChanges

Add utilities for creating SourceChanges

R=scheglov@google.com

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

[Paul Berry, 2015-04-27 16:30:09]

paulberry@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2015-04-27 16:30:10]

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
File pkg/analysis_server/lib/utilities/change_builder_core.dart (right):

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
pkg/analysis_server/lib/utilities/change_builder_core.dart:79: * Add the region
of text starting at the given [offset] and continuing for
Since this class is used to build up a change to a file, we need to clarify
whether the offset and length are relative to the original state of the
document, the state of the document as changed so far, or the state of the
document as it will appear once all the edits have been applied.  (I suspect
it's relative to the state of the document as it will appear once all the edits
have been applied, but I haven't worked with the underlying code much so I'm not
certain).

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
pkg/analysis_server/lib/utilities/change_builder_core.dart:86: * Add a
replacement of text starting at the given [offset] and continuing for
Similarly, it's not clear here whether the offset and length are relative to the
original state of the document or the state of the document as modified so far. 
(I suspect it's relative to the original state of the document).

Perhaps an example would be helpful.

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
pkg/analysis_server/lib/utilities/change_builder_core.dart:108: void
addSuggestions(LinkedEditSuggestionKind kind, List<String> values);
s/List/Iterable/ so that the caller doesn't have to make unnecessary calls to
".toList()".

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
File pkg/analysis_server/lib/utilities/change_builder_dart.dart (right):

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
pkg/analysis_server/lib/utilities/change_builder_dart.dart:28: * Clients are not
expected to subtype this class.
It would be nice if we could find a way to make the methods in this class more
composable.  E.g. let's say I want to insert the class declaration:

class C {
  C();

  @override
  inheritedMember() { ... }
}

It would be nice if I could do something like this:

ExecutableElement inheritedMember = ...;
writeClassDeclaration('C', members: (ClassElement element) {
  writeConstructorDeclaration(element);
  writeOverrideOfInheritedMember(inheritedMember);
});

[Brian Wilkerson, 2015-04-28 15:17:00]

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
File pkg/analysis_server/lib/utilities/change_builder_core.dart (right):

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
pkg/analysis_server/lib/utilities/change_builder_core.dart:79: * Add the region
of text starting at the given [offset] and continuing for
> ... we need to clarify
> whether the offset and length are relative to the original state of the
> document, the state of the document as changed so far, or the state of the
> document as it will appear once all the edits have been applied.

Good point. I'll work on improving the docs.

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
pkg/analysis_server/lib/utilities/change_builder_core.dart:86: * Add a
replacement of text starting at the given [offset] and continuing for
Ditto

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
pkg/analysis_server/lib/utilities/change_builder_core.dart:108: void
addSuggestions(LinkedEditSuggestionKind kind, List<String> values);
Done

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
File pkg/analysis_server/lib/utilities/change_builder_dart.dart (right):

https://codereview.chromium.org/1110583002/diff/1/pkg/analysis_server/lib/uti...
pkg/analysis_server/lib/utilities/change_builder_dart.dart:28: * Clients are not
expected to subtype this class.
I like that idea! I'm not sure what the signature of the closure should look
like, so I'll wait a while before adding it, but I'll add a comment so that I
don't forget to come back to this.

[Brian Wilkerson, 2015-04-29 18:38:54]

brianwilkerson@google.com changed reviewers:
+ scheglov@google.com

[Brian Wilkerson, 2015-04-29 18:38:54]

I've made a few changes, added an implementation, and added tests. PTAL.

[scheglov, 2015-04-30 04:35:36]

LGTM

[Brian Wilkerson, 2015-04-30 14:57:11]

Committed patchset #3 (id:40001) manually as 45461 (presubmit successful).

[Paul Berry, 2015-05-04 16:00:29]

Sorry for the slow response.  Hopefully these comments aren't too late to be
useful.

https://codereview.chromium.org/1110583002/diff/40001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/utilities/change_builder_core.dart (right):

https://codereview.chromium.org/1110583002/diff/40001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/utilities/change_builder_core.dart:70:
LinkedEditGroup group = _linkedEditGroups[groupName];
This could be written more compactly as:

return _linkedEditGroups.putIfAbsent(groupName, () => new
LinkedEditGroup.empty());

https://codereview.chromium.org/1110583002/diff/40001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/utilities/change_builder_dart.dart (right):

https://codereview.chromium.org/1110583002/diff/40001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/utilities/change_builder_dart.dart:47: static const
String ABSTRACT_MODIFIER = 'abstract ';
Personally, I'm a little dubious about making strings like 'abstract ', 'const
', and so on constants rather than just typing them directly at the site where
they're used.  Normally the benefits of making constants are for terseness or
readability, to allow searching for uses, to provide a convenient place to
document the constant, or to make it easy to change the constant if necessary in
the future.  None of those benefits seem to apply in this situation.

It also seems a little inconsistent that we have constants for 'abstract ' and
'const ' but a lot of other strings are just literal at the usage site like
'class ', ' extends ', ' with ', ' implements ', and so on.

Still, it's largely a matter of personal preference, so your call.

https://codereview.chromium.org/1110583002/diff/40001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/utilities/change_builder_dart.dart:94: if
(superclass != null) {
Two things:

1. If superclass == null && mixins.isNotEmpty, then an "extends" clause is
required, so we should output "extends Object"

2. If mixins.isEmpty && superclass.isObject, we might want to consider
suppressing the "extends" clause because it's redundant.

https://codereview.chromium.org/1110583002/diff/40001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/utilities/change_builder_core.dart (right):

https://codereview.chromium.org/1110583002/diff/40001/pkg/analysis_server/lib...
pkg/analysis_server/lib/utilities/change_builder_core.dart:82: * The [offset] is
relative to the original source. This is typically used to
This seems incorrect to me.  The implementation of addLinkedPosition uses the
offset to populate a Position in a LinkedEditGroup.  According to the API
specification, a LinkedEditGroup is:

A collection of positions that should be linked (edited simultaneously) for the
purposes of updating code after a source change. For example, if a set of edits
introduced a new variable name, the group would contain all of the positions of
the variable name so that if the client wanted to let the user edit the variable
name after the operation, all occurrences of the name could be edited
simultaneously.

So it seems to me that the offset has to be relative to the state of the
document as it will appear after all the edits have been applied.  Otherwise it
couldn't possibly refer to the positions of variable names that have been
introduced.  Am I misunderstanding?