Addition of the new flag "file-read-mode" into the analysis server to fix the offset bug with Intel…

Addition of the new flag "file-read-mode" into the analysis server to fix the offset bug with IntelliJs integration.

R=brianwilkerson@google.com, paulberry@google.com

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

[jwren, 2015-01-20 18:30:48]

jwren@google.com changed reviewers:
+ brianwilkerson@google.com, paulberry@google.com

[Brian Wilkerson, 2015-01-20 18:56:14]

LGTM, once comments are addressed

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/lib/file_system...
File pkg/analyzer/lib/file_system/physical_file_system.dart (right):

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/lib/file_system...
pkg/analyzer/lib/file_system/physical_file_system.dart:23: static final
NORMALIZE_EOL_MODE =
Unless I missed something, there doesn't appear to be a test for this function.

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/test/generated/...
File pkg/analyzer/test/generated/all_the_rest_test.dart (right):

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/test/generated/...
pkg/analyzer/test/generated/all_the_rest_test.dart:7387: void
test_fileReadMode() {
We should also have a test like:
   expect(FileBasedSource.fileReadMode('a\r\nb'), 'a\r\nb');

[Paul Berry, 2015-01-20 19:10:53]

lgtm once '\r' -> '\n' behavior is addressed.  Your call on everything else.

https://codereview.chromium.org/857283003/diff/1/pkg/analysis_server/doc/api....
File pkg/analysis_server/doc/api.html (right):

https://codereview.chromium.org/857283003/diff/1/pkg/analysis_server/doc/api....
pkg/analysis_server/doc/api.html:182: can also be set to <tt>normalize-eol</tt>.
This documentation should precisely define how normalize-eol behaves, since (as
we observed by looking at emacs) different editors normalize eol's in different
ways.  In particular, it should be clear that:

- '\r\n' is converted to '\n'.
- '\r' by itself is converted to '\n'.
- this happens regardless of the OS editor is running on.

Also, given what we learned about emacs, you might want to consider renaming
"normalize-eol" to something like "normalize-eol-always", since in the future
we'll need to add a mode for emacs that normalizes only normalizes some eol's
under certain specific conditions.

https://codereview.chromium.org/857283003/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/socket_server.dart (right):

https://codereview.chromium.org/857283003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/socket_server.dart:68: } else {
I would feel a lot more comfortable if we changed this to:

...
} else  if (analysisServerOptions.fileReadMode == 'as-is') {
  resourceProvider = PhysicalResourceProvider.INSTANCE;
} else {
  ... throw an exception ...
}

just so that we don't have an implicit dependency on code in some other file to
validate the string for us.

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/lib/file_system...
File pkg/analyzer/lib/file_system/physical_file_system.dart (right):

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/lib/file_system...
pkg/analyzer/lib/file_system/physical_file_system.dart:24: (String string) =>
string.replaceAll('\r\n', '\n');
Based on previous email discussions, I believe there is an additional behavior
we need: any '\r' *not* followed by '\n' needs to be converted into '\n'.

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/lib/file_system...
pkg/analyzer/lib/file_system/physical_file_system.dart:38:
PhysicalResourceProvider._(String fileReadMode(String s)) {
Rather than having a private constructor and two instances (which limits all
analyzer clients to just the two file read modes) consider making this
constructor public and allow the analyzer client to specify the conversion
function.  We could still provide a static instance for the convenience of
clients that just want the as-is behavior (e.g. tests), but clients that wanted
to specify their own file read behavior would do so by constructing a
PhysicalResourceProvider and passing in a function.

This would have the advantage that in the future when we need to add additional
file read modes, we would only have to change analysis server; we would not need
to rev analyzer to support the new mode.

[jwren, 2015-01-20 21:38:14]

https://codereview.chromium.org/857283003/diff/1/pkg/analysis_server/doc/api....
File pkg/analysis_server/doc/api.html (right):

https://codereview.chromium.org/857283003/diff/1/pkg/analysis_server/doc/api....
pkg/analysis_server/doc/api.html:182: can also be set to <tt>normalize-eol</tt>.
On 2015/01/20 19:10:53, Paul Berry wrote:
> This documentation should precisely define how normalize-eol behaves, since
(as
> we observed by looking at emacs) different editors normalize eol's in
different
> ways.  In particular, it should be clear that:
> 
> - '\r\n' is converted to '\n'.
> - '\r' by itself is converted to '\n'.
> - this happens regardless of the OS editor is running on.
> 
> Also, given what we learned about emacs, you might want to consider renaming
> "normalize-eol" to something like "normalize-eol-always", since in the future
> we'll need to add a mode for emacs that normalizes only normalizes some eol's
> under certain specific conditions.

Done.

https://codereview.chromium.org/857283003/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/socket_server.dart (right):

https://codereview.chromium.org/857283003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/socket_server.dart:68: } else {
On 2015/01/20 19:10:53, Paul Berry wrote:
> I would feel a lot more comfortable if we changed this to:
> 
> ...
> } else  if (analysisServerOptions.fileReadMode == 'as-is') {
>   resourceProvider = PhysicalResourceProvider.INSTANCE;
> } else {
>   ... throw an exception ...
> }
> 
> just so that we don't have an implicit dependency on code in some other file
to
> validate the string for us.

Done.

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/lib/file_system...
File pkg/analyzer/lib/file_system/physical_file_system.dart (right):

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/lib/file_system...
pkg/analyzer/lib/file_system/physical_file_system.dart:23: static final
NORMALIZE_EOL_MODE =
On 2015/01/20 18:56:14, Brian Wilkerson wrote:
> Unless I missed something, there doesn't appear to be a test for this
function.

Done.

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/lib/file_system...
pkg/analyzer/lib/file_system/physical_file_system.dart:24: (String string) =>
string.replaceAll('\r\n', '\n');
Done.  As we discussed, I remembered this but left it out.  Including the \r
->\n now.

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/lib/file_system...
pkg/analyzer/lib/file_system/physical_file_system.dart:38:
PhysicalResourceProvider._(String fileReadMode(String s)) {
On 2015/01/20 19:10:53, Paul Berry wrote:
> Rather than having a private constructor and two instances (which limits all
> analyzer clients to just the two file read modes) consider making this
> constructor public and allow the analyzer client to specify the conversion
> function.  We could still provide a static instance for the convenience of
> clients that just want the as-is behavior (e.g. tests), but clients that
wanted
> to specify their own file read behavior would do so by constructing a
> PhysicalResourceProvider and passing in a function.
> 
> This would have the advantage that in the future when we need to add
additional
> file read modes, we would only have to change analysis server; we would not
need
> to rev analyzer to support the new mode.

Done.

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/test/generated/...
File pkg/analyzer/test/generated/all_the_rest_test.dart (right):

https://codereview.chromium.org/857283003/diff/1/pkg/analyzer/test/generated/...
pkg/analyzer/test/generated/all_the_rest_test.dart:7387: void
test_fileReadMode() {
On 2015/01/20 18:56:14, Brian Wilkerson wrote:
> We should also have a test like:
>    expect(FileBasedSource.fileReadMode('a\r\nb'), 'a\r\nb');

Done.

[jwren, 2015-01-20 21:48:18]

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