This simply does what any other Dart library is doing, e.g. dart:io does this too.

Does this change also prevent "Not enabling colors, stdout does not support ANSI colors." from being emitted?

@zanderso no

Why didn't this warning appear for us? We should make sure our pipeline is reporting all issues (including nits).

Also, why does dart:_internal export this separately from dart:core?

@Hixie it's two different classes. dart:core.Symbol is interface and dart:_interal.Symbol is a hidden implementation of dart:core.Symbol. I will clarify the commit message.

@Hixie as for the first question, I don't know - it requires somebody with knowledge in both analyzer and fasta to answer why you don't get the same warning with your normal tools.

Fasta is strictly follows the spec. Maybe analyzer is more lax?

/cc @peter-ahe-google @lrh @stereotype441

This is not a warning, it's an optional hint (which we call a "nit"). It's normally disabled, but it is possible that it was enabled by accident when we build platform.dill.

Regardless, this change should land because it can lead to confusion.

I also verified that this change makes engine build completely silent.

@Hixie can somebody press merge button for me? I don't have commit rights to this repo.

Before we land this we should make sure we are reporting all issues (including nits) for all Dart code that is in the Flutter engine repo in the Travis tests (at least, all the ones we enable for the Flutter repo itself, I guess there's some messages we want to intentionally omit because we know they give false positives). Otherwise we have no way to know when we regress it. Or in other words, "this needs a test".

@peter-ahe-google errors, warnings, nits, lints, and hints are all the same as far as we're concerned. The goal is just to make sure the code is correct.

I would also suggest we look into why we're exporting this symbol twice. This seems like an actual error that we should fix, rather than just silencing it.

@Hixie I explained why we are exporting it twice in this comment -> #3861 (comment), it's two different classes named in the same way.

Ah, sorry, missed that. Should the internal one be renamed maybe? To avoid the clash? e.g. to _Symbol?

It can not be renamed to _Symbol because it will become inaccessible to dart:core so the redirection in dart:core will become illegal -> https://github.com/dart-lang/sdk/blob/master/sdk/lib/core/symbol.dart#L37

It could be renamed to something like SymbolImpl I guess, but renaming classes in core libraries is something that probably requires a much bigger change than adding hide Symbol here.

/cc @lrhn Lasse, do you think we can eventually rename Symbol in dart:_internal to something that does not cause clashes (e.g. SymbolImpl)?

If the conflict is on purpose and can't be fixed then hiding the symbol seems like a reasonable workaround.
LGTM modulo a test (that is, enabling whatever lints and so forth are necessary to make sure we would have caught this and won't regress this again).

I added Symbol to dart:_internal originally. The conflict was deliberate to ensure that #foo.runtimeType.toString() would evaluate to "Symbol".

@peter-ahe-google thanks for the context, Peter! Makes sense.

@Hixie the "test" part is handled as part of dart-lang/sdk#30127 - Peter is updating patch_sdk tool. We will make sure that any compile time message printed during build is also reflected in the exitCode - so you either get a successful and silent build or a noisy and unsuccessful one.

Will that only trigger for fuchsia builds or will it report problems for engine builds on all platforms?

We run the analyzer on dart:ui on Travis. We don't run the build on Travis. If there's a way to catch it on Travis that would certainly be better.

@mraleph Looks like that bug is fixed now, but nothing broke on our end, so we're not testing enough. This needs something that would have made Travis go red, to ensure that we don't regress it unknowingly.

@mraleph Are you still actively working on this PR?

I think this PR was in the "ready-to-land-just-waiting-for-lgtm" stage for the last 22 days.

Well ideally it would have some sort of test to catch regressions. See my July 11 10:43am comment.

After further discussion offline, we concluded that it wasn't a big deal if it regressed for now and eventually if it regressed it would break the build so we would know.