Sdk: Don't allow "ignore:" to suppress errors

[stereotype441]

The "ignore:" syntax was intended to allow users to suppress warnings, hints, and lints; and nearly all cases that is how it is used. However, it currently allows errors to be suppressed as well. This creates the risk that genuine errors will creep into users' code bases due to inadvertent use of "ignore:". We would like to change the behavior of "ignore:" so that it cannot suppress errors.

Note that before we can fix this we must address https://github.com/dart-lang/sdk/issues/26980 (since Flutter is currently using the "ignore:" syntax to work around that issue). To work around this issue, we plan to change analyzer's behavior to match the VM's (so that enums may be equality compared inside constants). The issue will remain open, since the spec either needs to be updated to match this behavior, or we need to change the VM and analyzer to disallow it and find a different way to meet Flutter's needs.

[pq]

So this may be contentious (and I'm sorry it's taken me over a year to discover this) but I'm inclined to say this is working very much by design and provides a pretty valuable escape hatch for folks like Flutter and anyone playing with bleeding edge functionality.

If this is something we wanted to enforce tactically, I'd suggest creating a lint that flags ignored errors...

@stereotype441 : how does that sound?

[zoechi]

I also think this is problematic, especially with code generation which is now widely used
where for example non-existing identifiers are imported which causes errors.
This might look differently when the analyzer supports generated code in .dart-tool/

[matanlurey]

Errors are things that would prevent compilation.

[natebosch]

I think anything that is going to blow up in the CFE should not be ignorable - can we revisit this?

[srawlins]

Still blocked on #26980; flutter ignores CONST_EVAL_THROWS_EXCEPTION.

[pq]

To reiterate, I think the feature is working as intended as is and would push back against changing it. In practice, I don't see the benefit in making the mechanism more complex. Are there any motivating examples?

[matanlurey]

It made sense in Dart 1 where compile-time errors could be ignored (in some cases), or there were bugs in what the analyzer and the runtimes (Dart2JS or the VM) considered an error - i.e. the analyzer was sometimes stricter than the language. I see https://github.com/dart-lang/sdk/issues/26980 is still an example of that.

For example, there is _no_ value in the following:

void main() {
  // ignore: invalid_assignment
  String x = 5;
}

Your code cannot compile. CFE will fail, as well all the tools that use it.

Imagine you could write:

// ignore: invalid_syntax
this + is - not ~ dart * code() [[[]]]

What would be the value in allowing a user to ignore syntax errors?

One problematic aspect is that ignore: xxx can be used ignore false-positive hints/lints/bugs which do not affect the compilers or CFE, but we don't want to train users they can ignore: xxx anything they want (i.e. compile-time errors).

[matanlurey]

(If we decide to close this issue, we would likely ban using // ignore: xxx internally at all)

[matanlurey]

/cc @MichaelRFairhurst who we had a long-ish conversation with about this meta issue.

[MichaelRFairhurst]

Yeah I support this change.

The CFE will not honor it.

While we're at it, I think we should remove the ability to remap error
codes. (Changing warnings and hints and lint's should be fine).

I suppose if we wanted to really make this long tail, we should look at
whether analysis_options.yaml somehow coordinate with the CFE, so that
things like --no-raw-types or crazier things like --invariant-generics
would be understood by both and not one or the other, and warnings mapped
to errors would be errors in both worlds.

On Aug 23, 2018 3:14 PM, "Matan Lurey" [e-mail] wrote:

/cc @MichaelRFairhurst https://github.com/MichaelRFairhurst who we had a
long-ish conversation with about this meta issue.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/dart-lang/sdk/issues/27218#issuecomment-415573270, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABjWe85SDt_xUWnUu3lBR8huVBHh_p2-ks5uTxs-gaJpZM4Jy4_o
.

[pq]

Sorry to keep harping on this, but I have to ask...

While we're at it, I think we should remove the ability to remap error
codes. (Changing warnings and hints and lint's should be fine).

Is this a matter of taste or something that is negatively impacting folks in practice?

[MichaelRFairhurst]

I think it's three things:

• preference (I don't like unsafe things that aren't necessary, I don't
  need an example to not like it)
• polish/consistency (it's only WAI until you try to run the code..! So, is
  this behavior "honest"?)
• outdated (errors are no longer things we can casually brush aside)

Is it a lot of work?

On Thu, Aug 23, 2018, 3:46 PM Phil Quitslund [e-mail]
wrote:

Sorry to keep harping on this, but I have to ask...

While we're at it, I think we should remove the ability to remap error
codes. (Changing warnings and hints and lint's should be fine).

Is this a matter of taste or something that is negatively impacting folks
in practice?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/dart-lang/sdk/issues/27218#issuecomment-415581693,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABjWe3oVaz4zyy555KGRVcApSJwSGzJMks5uTyLQgaJpZM4Jy4_o
.

[pq]

• outdated (errors are no longer things we can casually brush aside)

I don't know if this was ever true to be honest but I hear what you're saying.

Anyway, in the end, I think we just disagree. If this is something that folks strongly want to pursue, I'm happy to work a bit on persuading you. Maybe best in person though... And ideally over beer!

🍻

[MichaelRFairhurst]

Seems a great occasion for a beer to me :)

I'll also say, from the purely pragmatic angle (the one I think you're taking, Phil), the fact that the CFE will always detect this case is a sign that arguably its _not_ an issue.

For instance, if we ran an automated LSC in g3 to add + ignore a bunch of errors everywhere.....it would all fail on TAP anyway!

So the fact that it fails anyway on the CFE is arguably a case to why we should just use this internally and its _not_ a problem?

[devoncarew]

It looks like there is an least one use of the ability to ignore errors in the Dart SDK, and ~5 uses in Flutter:

• https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8888639769928776240/+/steps/analyze_pkg_dart_internal/0/stdout
• https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8888639769928776224/+/steps/analyze_flutter/0/stdout

[devoncarew]

Here's a POC of changing to not allowing error severity items to be ignored: https://dart-review.googlesource.com/c/sdk/+/135441 .

In order to do that, we'd need to address:

• the single special case in the dart sdk repo - package:dart_internal needs to be able to continue ignoring the error related to importing dart:_internal.

• check for any uses in google3 and handle them

• address one issue in the flutter sdk related to analyzing generated code that does not yet exist (https://github.com/flutter/flutter/blob/master/dev/integration_tests/codegen/lib/main.dart#L7)

• address 4 items in the flutter repo where some symbols referenced in dart:ui only exist in the web version of dart:ui (and we analyze against the mobile version). Those probably need to be unwound so they're not special cased - have some other mechanism to access them, or add them to both the web and mobile version of dart:ui. These items were added in https://github.com/flutter/flutter/pull/42689 and https://github.com/flutter/flutter/pull/39628 (webOnlyInstantiateImageCodecFromUrl(), webOnlySetPluginHandler(), and debugEmulateFlutterTesterEnvironment).

cc @stereotype441, @matanlurey, @srawlins

[pq]

I haven't looked too closely at this so I may be off but I wonder if this couldn't be subverted using the mechanism for changing severities in options:

analyzer:
  errors:
    invalid_assignment: warning

(And if so, is that a problem?)

[matanlurey]

I wonder if this couldn't be subverted using the mechanism...

At least in google3, you would not be able to. So that passes our requirements 👍

(And might let Flutter downgrade errors -> warnings where they see fit?)

[Hixie]

The analyzer does not know everything. It might be used to analyze intentionally-incomplete code, it might be used to analyze code with known-broken dependencies, it might be used to analyze code for a future version of Dart, or any number of other situations where the developer knows better than the analyzer.

Philosophically, the analyzer is a tool. This is analogous to making a hammer which refused to hit screws. Even the SawStop comes with a bypass mode.

[srawlins]

I've opened an issue to consider reverting this behavior. https://github.com/dart-lang/sdk/issues/44237

[pq]

Excellent. Thanks @srawlins!