SDK now refuses to compile code with invalid annotations. Breaks code that uses unittest.

[alan-knight]

As of https://codereview.chromium.org/1510863004 dart2js does not compile code that includes an invalid annotation. package:unittest/unittest.dart has an invalid annotation (@deprecated(...)), so things that still uses unittest refuse to compile. This is a badly breaking change, and the only reason the bots are still working is that the SDK has an old version of unittest without that annotation.
@kevmoo @johnniwinther

[dgrove]

@sigmundch

[alan-knight]

Right now, this would break many applications. For example, if we updated the SDK it would break every html test and various package tests.

It would be easy to fix unittest, but we would still break things until people did a pub upgrade.

I think it would be better if we made this a warning in the compiler where it would complain, but not refuse to compile.

[sigmundch]

Section 15 of the spec does indicate that it should be a compile-time error though. @gbracha can we change it to be just a warning in this case?

The alternative I see is for us to make it a warning in Dart 1.14, update all the packages, and make it an error in Dart 1.15

[sigmundch]

(just to clarify - the CL above was fixing #14548 - a very old dart2js bug regarding that spec section)

[kevmoo]

Let's not change the spec. This is totally valid.

But waiting at least until 1.15 to crash in dart2js is a good idea, IMHO

[lrhn]

Also, take this as a good time to switch to package:test.

[johnniwinther]

Dart2js doesn't crash; it reports a compile-time error.

On 10 December 2015 at 02:30, Kevin Moore [email protected] wrote:

Let's not change the spec. This is totally valid.

But waiting at least until 1.15 to crash in dart2js is a good idea, IMHO

—
Reply to this email directly or view it on GitHub
#25204 (comment).

[sigmundch]

@johnniwinther - I think @kevmoo meant "crash" as "produce an error without any output".

@floitschG gave a couple additional suggestions:

• emit an error message in dart2js but still continue. Make it fatal sometime in the future.
• abort the compilation, but provide a flag to ignore that error. Remove that flag sometime in the future.

I'm fine with either - @johnniwinther which of these would be easier? Seems like the latter might fit more nicely with our current architecture.

[kevmoo]

Folks are going to be annoyed enough w/ new error messages.

NO FLAGS for this. The result should NOT CHANGE from before. Yell at the user: fine. 😃

[nex3]

package:unittest/unittest.dart has an invalid annotation (@deprecated(...))

Where is this annotation? I can't find it in the 0.11.x branch or indeed anywhere in the history of the repo.

[alan-knight]

https://github.com/dart-lang/old_unittest/blob/master/lib/unittest.dart

On Thu, Dec 10, 2015 at 3:15 PM Natalie Weizenbaum [email protected]
wrote:

package:unittest/unittest.dart has an invalid annotation (@deprecated
https://github.com/deprecated(...))

Where is this annotation? I can't find it in the 0.11.x branch
https://github.com/dart-lang/test/tree/0.11.x or indeed anywhere in the
history of the repo.

—
Reply to this email directly or view it on GitHub
#25204 (comment).

[nex3]

I see. That repo tracks the 0.12.x releases of the unittest package; there should be no reason to use it anymore, since it just exports test. It also has an implicit SDK upper bound of 0.12.4, so how is this issue even coming up?

[kevmoo]

@alan-knight which version of unittest are you seeing the issue with?

[alan-knight]

I saw this because I have a package that I just test various small things
in. It has unittest: any in the pubspec. The pubspec.lock has

unittest:
description: unittest
source: hosted
version: "0.12.4"

On Thu, Dec 10, 2015 at 3:56 PM Kevin Moore [email protected]
wrote:

@alan-knight https://github.com/alan-knight which version of unittest
are you seeing the issue with?

—
Reply to this email directly or view it on GitHub
#25204 (comment).

[johnniwinther]

@sigmundch We already have --generate-code-with-compile-time-errors option. Is that sufficient?

[kevmoo]

@johnniwinther Please don't require a new flag to compile code that has compiled fine for a year.

Let's go w/ a warning.

[sigmundch]

keeping it as a warning for now sounds good.

If we want to really be spec compliant, we could report it as an error, but continue compiling for this specific error only. That would require that we divide errors in 2 groups (fatal and non-fatal). Feels odd because all errors except this one would be fatal, but we could use it as a way of introducing errors in the future. Non-fatal errors will have a date/release when they will be come fatal (e.g. say that in 1.15 we'll flip the bit), so users have a transition path.

[johnniwinther]

@alan-knight I don't see how the deprecated(...) code was affected by the mentioned CL. Running dart2js on

@deprecated(
    'The "unittest" package is deprecated. Use the "test" package instead.')
main() {}

gives the following result on e03c395 (the previous commit):
temp/test4.dart:2:2:
Error: 'deprecated' is not a type.
@deprecated(
^^^^^^^^^^
temp/test4.dart:2:2:
Error: Not a compile-time constant.
@deprecated(
'The "unittest" package is deprecated. Use the "test" package instead.')

temp/test4.dart:2:2:
Error: Not a compile-time constant.
@deprecated(
'The "unittest" package is deprecated. Use the "test" package instead.')

Error: Compilation failed.

Do you have an example of a program with this annotation that worked before but not after?

[alan-knight]

Yes, I see that as well. So it must have been a pub upgrade giving me the version with the invalid annotation that caused it, rather than the compiler change.

[kevmoo]

@alan-knight – @nex3 did fix pkg/unittest, too.