Breaking changes in 'analyzer' package.

Breaking changes in 'analyzer' package.

1. Source.uri added.

2. DartSdk.fromEncoding() replaced with fromFileUri().

  This CL cannot be committed as is until pkg/code_transformers is fixed.

R=brianwilkerson@google.com, sigmund@google.com
BUG=

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

[Brian Wilkerson, 2014-08-04 20:40:04]

LGTM

[Siggi Cherem (dart-lang), 2014-08-04 22:43:55]

Besides the code_transformers tests, I'd recommend also running tests under
polymer/test, especially those under polymer/test/build tests (there is some
good extra coverage there).

That would give you some assurance that the changes are working OK.

https://codereview.chromium.org/428303004/diff/1/pkg/code_transformers/lib/sr...
File pkg/code_transformers/lib/src/resolver_impl.dart (right):

https://codereview.chromium.org/428303004/diff/1/pkg/code_transformers/lib/sr...
pkg/code_transformers/lib/src/resolver_impl.dart:119: // TODO(scheglov) how to
get Uri here?
You might be able to basically use the logic we have in
_AssetBasedSource.getSourceUri

one question I have is what is this Uri supposed to look like? Is it supposed to
be the fully resolved Uri, the string was found when loading this asset in the
first place?

https://codereview.chromium.org/428303004/diff/1/pkg/code_transformers/lib/sr...
pkg/code_transformers/lib/src/resolver_impl.dart:439: source = new
_AssetBasedSource(assetId, _resolver, uri);
if this is intended to be the resolved uri, then I wonder is 'uri' guaranteed to
be resolved at this point, or should we also use getSourceUri here?

In that case, I'd remove the constructor argument and set 'uri' internaly via
getSourceUri.

[blois, 2014-08-05 21:33:26]

https://codereview.chromium.org/428303004/diff/1/pkg/code_transformers/lib/sr...
File pkg/code_transformers/lib/src/dart_sdk.dart (right):

https://codereview.chromium.org/428303004/diff/1/pkg/code_transformers/lib/sr...
pkg/code_transformers/lib/src/dart_sdk.dart:114: return
_proxy.uri.resolveUri(relativeUri);
The DartSourceProxy wraps 'dart:' sources, and I'm seeing that _proxy.uri is
'dart:core', which is incorrectly resolving 'annotation.dart' to be:
'dart:annotation.dart'.

I think that the 'uri' property is incredibly misleading, in this case (and in
others).

The problem is that we have URIs which can be resolved relative to something,
but not absolutely. This is one example, but another one is files in packages
not in the lib/ folder. These are only accessible via relative URIs by other
files within that package.

Because Source.uri is not something that can be used as the import URI, I do not
think it should use the 'uri' name.

[Brian Wilkerson, 2014-08-05 21:44:21]

https://codereview.chromium.org/428303004/diff/1/pkg/code_transformers/lib/sr...
File pkg/code_transformers/lib/src/dart_sdk.dart (right):

https://codereview.chromium.org/428303004/diff/1/pkg/code_transformers/lib/sr...
pkg/code_transformers/lib/src/dart_sdk.dart:114: return
_proxy.uri.resolveUri(relativeUri);
> The DartSourceProxy wraps 'dart:' sources, and I'm seeing that _proxy.uri is
> 'dart:core', which is incorrectly resolving 'annotation.dart' to be:
> 'dart:annotation.dart'.

That's a bug. It should be resolved to dart:core/annotation.dart. There is a
test for this, at least on the Java side.
 
> I think that the 'uri' property is incredibly misleading, in this case (and in
> others).
> 
> The problem is that we have URIs which can be resolved relative to something,
> but not absolutely. This is one example, but another one is files in packages
> not in the lib/ folder. These are only accessible via relative URIs by other
> files within that package.

The uri field should always be the absolute URI resulting from resolving the URI
in the directive against the absolute URI of the file containing the directive.

> Because Source.uri is not something that can be used as the import URI, I do
not
> think it should use the 'uri' name.

It won't always be the same as the original (possibly relative) URI in the
directive, but it should be a valid absolute URI that references the same file
(and hence could replace the import URI).

[blois, 2014-08-05 22:28:50]

Updates are at https://codereview.chromium.org/447603002/

https://codereview.chromium.org/428303004/diff/1/pkg/analyzer/lib/src/generat...
File pkg/analyzer/lib/src/generated/source.dart (right):

https://codereview.chromium.org/428303004/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/source.dart:600: } catch (exception) {
exception may not be a subclass of Exception. logError2 expects it to be,
causing an issue in checked mode.

This seems to occur across the board, and makes debugging errors unfun.

[scheglov, 2014-08-06 02:12:00]

PTAL

I merged https://codereview.chromium.org/447603002/ into this one.
Also updated versions for analyzer, code_transformers and smoke.

Smoke tests are failing, an issue was opened.

[Brian Wilkerson, 2014-08-06 02:21:41]

LGTM

[Siggi Cherem (dart-lang), 2014-08-06 03:07:22]

https://codereview.chromium.org/428303004/diff/20001/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/dart_sdk.dart (right):

https://codereview.chromium.org/428303004/diff/20001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/dart_sdk.dart:113: print('$relativeUri => $r      
    using $_proxy');
delete print?

https://codereview.chromium.org/428303004/diff/20001/pkg/code_transformers/pu...
File pkg/code_transformers/pubspec.yaml (right):

https://codereview.chromium.org/428303004/diff/20001/pkg/code_transformers/pu...
pkg/code_transformers/pubspec.yaml:2: version: 0.2.0+4
if we don't intend to release this immediately, this should change to
'0.2.1-dev'

https://codereview.chromium.org/428303004/diff/20001/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/428303004/diff/20001/pkg/pkg.status#newcode66
pkg/pkg.status:66: smoke/test/codegen/recorder_test: Fail # Issue 20379
Like with code_transformers, I'd prefer to update smoke as well. If these tests
are failing, I'd be very skeptical of releasing smoke until they get fixed.

I patched the code and it turns out that the fix is very simple. You can copy
the changes from here: https://codereview.chromium.org/444843003.

https://codereview.chromium.org/428303004/diff/20001/pkg/smoke/pubspec.yaml
File pkg/smoke/pubspec.yaml (right):

https://codereview.chromium.org/428303004/diff/20001/pkg/smoke/pubspec.yaml#n...
pkg/smoke/pubspec.yaml:2: version: 0.2.0+4
here too - 0.2.1-dev

https://codereview.chromium.org/428303004/diff/20001/pkg/smoke/pubspec.yaml#n...
pkg/smoke/pubspec.yaml:12: analyzer: ">=0.22.0 <0.23.0"
mmm, this is a bit unfortunate, since public APIs from smoke don't break, only
tests are affected. While technically that is still a break, our public code
still supports using analyzer >=0.13.0 < 0.23.0.

This might not be worth it, but is it possible to make this a backwards
compatible change? For example, if we add the API with a different name
(resolveRelativeUri) and leave the old API unused, we would still have a
breaking change, but it would let us have smoke and code_transformers continue
to work with old versions of the analyzer at the same time.

Thoughts?

[vicb, 2014-08-06 08:37:07]

Konstantion,

Could you please add a CHANGELOG.md as discussed earlier.

It is very important for the consumers of this lib.

Thanks,
Victor

[scheglov, 2014-08-06 18:03:50]

PTAL

https://codereview.chromium.org/428303004/diff/20001/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/dart_sdk.dart (right):

https://codereview.chromium.org/428303004/diff/20001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/dart_sdk.dart:113: print('$relativeUri => $r      
    using $_proxy');
On 2014/08/06 03:07:22, Siggi Cherem (dart-lang) wrote:
> delete print?

Done.

https://codereview.chromium.org/428303004/diff/20001/pkg/code_transformers/pu...
File pkg/code_transformers/pubspec.yaml (right):

https://codereview.chromium.org/428303004/diff/20001/pkg/code_transformers/pu...
pkg/code_transformers/pubspec.yaml:2: version: 0.2.0+4
On 2014/08/06 03:07:22, Siggi Cherem (dart-lang) wrote:
> if we don't intend to release this immediately, this should change to
> '0.2.1-dev'

Done.

https://codereview.chromium.org/428303004/diff/20001/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/428303004/diff/20001/pkg/pkg.status#newcode66
pkg/pkg.status:66: smoke/test/codegen/recorder_test: Fail # Issue 20379
On 2014/08/06 03:07:22, Siggi Cherem (dart-lang) wrote:
> Like with code_transformers, I'd prefer to update smoke as well. If these
tests
> are failing, I'd be very skeptical of releasing smoke until they get fixed.
> 
> I patched the code and it turns out that the fix is very simple. You can copy
> the changes from here: https://codereview.chromium.org/444843003.
> 

Thank you.
Patched.

https://codereview.chromium.org/428303004/diff/20001/pkg/smoke/pubspec.yaml
File pkg/smoke/pubspec.yaml (right):

https://codereview.chromium.org/428303004/diff/20001/pkg/smoke/pubspec.yaml#n...
pkg/smoke/pubspec.yaml:2: version: 0.2.0+4
On 2014/08/06 03:07:22, Siggi Cherem (dart-lang) wrote:
> here too - 0.2.1-dev

Done.

https://codereview.chromium.org/428303004/diff/20001/pkg/smoke/pubspec.yaml#n...
pkg/smoke/pubspec.yaml:12: analyzer: ">=0.22.0 <0.23.0"
On 2014/08/06 03:07:22, Siggi Cherem (dart-lang) wrote:
> mmm, this is a bit unfortunate, since public APIs from smoke don't break, only
> tests are affected. While technically that is still a break, our public code
> still supports using analyzer >=0.13.0 < 0.23.0.

  Done.

> 
> This might not be worth it, but is it possible to make this a backwards
> compatible change? For example, if we add the API with a different name
> (resolveRelativeUri) and leave the old API unused, we would still have a
> breaking change, but it would let us have smoke and code_transformers continue
> to work with old versions of the analyzer at the same time.
> 
> Thoughts?

  I will let Brian to decide this API question.

[Brian Wilkerson, 2014-08-06 18:17:03]

LGTM

[Siggi Cherem (dart-lang), 2014-08-06 20:24:23]

lgtm

I have a fix for the warnings that I can submit immediately afterwards (see
https://codereview.chromium.org/451493002/)

[scheglov, 2014-08-06 20:42:06]

Committed patchset #4 manually as 38949 (presubmit successful).

[nweiz, 2014-08-06 20:56:52]

It looks like you're making changes to analysis_server, analysis_services, and
analysis_testing without changing their pubspecs. Unless a repo package has a
"-dev" version, its contents should always exactly match the contents of the
same version uploaded to pub.dartlang.org.

What this means is that, if you modify analysis_server and its repo version is
0.1.0, you have two choices. Either you can bump the version (here it would be
to 0.1.0+1, since this isn't a user-visible change) and make a new release; or
you can make it a -dev version (in this case 0.1.1-dev). If you're planning on
making a bunch of changes in a row, the latter is probably what you want to do.