Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler should not print warnings #46264

Open
Hixie opened this issue Jun 4, 2021 · 93 comments
Open

Compiler should not print warnings #46264

Hixie opened this issue Jun 4, 2021 · 93 comments
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool.

Comments

@Hixie
Copy link
Contributor

Hixie commented Jun 4, 2021

When I compile this:

void main() {
  const bool test = true;
  print(test!); // ignore: unnecessary_non_null_assertion                                                                                         
}

...I get:

test.dart:3:9: Warning: Operand of null-aware operation '!' has type 'bool' which excludes null.
  print(test!); // ignore: unnecessary_non_null_assertion
	^
true

I'm happy for the analyzer to give the warning, but why is the compiler also doing so? And how do I silence it?

The context is that I have code (a Flutter plugin) that needs to compile against two versions of an API. In one version (Flutter stable) a particular field is nullable, in the more recent version (Flutter master), it's non-nullable. I don't mind having lots of // ignores in the code during the transition, but I don't want users to be faced with lots of warnings they can't do anything about when compiling their apps.

@lrhn
Copy link
Member

lrhn commented Jun 5, 2021

This warning is a language specification mandated warning, which is why all source tools implement it.
Not all source tools understand ignore comments, though.

We may want to make warnings ignorable in other tools too, or make other tools not print even spec mandated warnings (let the analyzer be the only tool which does warnings).

@Hixie
Copy link
Contributor Author

Hixie commented Jun 5, 2021

Yeah, IMHO we should change the spec here. From a developer experience perspective, the analyzer is where the warnings are useful and the compiler should just always be silent unless it really can't build the code, IMHO. It just makes sense from a developer's point of view to have one source of places for messages about how the code is wrong.

If for some reason we really need the compiler to show warnings, having it implement // ignore would be ok too I guess.

@jcollins-g
Copy link
Contributor

From a user perspective, we should consider that there are three, rather than two, cases where language warnings are relevant.

The first is when a user analyzes code statically via the analyzer.
The second is when a user compiles an application to an executable format without running it.
The third is when the user is running an application in such a way that it has the side effect of compiling it. I think this is the case @Hixie is talking about?

It is fine IMO to emit warnings in the first and second cases, but the third case should not emit non-fatal warnings, ever. This is because the user or other programs intended to interpret the stdout/stderr of the code, may not expect there to be warnings in the output because after all, the program didn't print them itself and ran just fine.

jcollins-macbookpro2:~ jcollins$ cat > foo.dart
void main() {
  const bool test = true;
  print(test!); // ignore: unnecessary_non_null_assertion                                                                                         
}
jcollins-macbookpro2:~ jcollins$ dart foo.dart
foo.dart:3:9: Warning: Operand of null-aware operation '!' has type 'bool' which excludes null.
  print(test!); // ignore: unnecessary_non_null_assertion                                                                                         
        ^
true
jcollins-macbookpro2:~ jcollins$ dart foo.dart 2>/dev/null
true

@devoncarew devoncarew added the area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. label Jun 7, 2021
@devoncarew
Copy link
Member

I'm triaging this to dart-dart-cli as I believe that the resolution - if that's what we agree on - would involve changes to dart run (both the implicit version and the explicit version). That is, the verbosity setting for compiling should default to 'all', while the verbosity setting for running should instead default to 'error'.

    --verbosity                                        Sets the verbosity level of the compilation.

          [all] (default)                              Show all messages
          [error]                                      Show only error messages
          [info]                                       Show error, warning, and info messages
          [warning]                                    Show only error and warning messages

cc @bkonyi

@Hixie
Copy link
Contributor Author

Hixie commented Jun 8, 2021

I don't really see why #2 should print warnings either. It doesn't print lints, and some of the "warnings" we print (e.g. that ! was used redundantly) are much less serious than some of the lints we don't print (e.g. no_logic_in_create_state).

When you're running flutter build (compiling), you're past the point where being told about a stray ! is really relevant. You've clearly decided that that's the way it's going to be, and you just want an APK or whatever.

That said, if there is a way to mute the compiler in general then I would be satisfied with just having the flutter tool always mute the compiler; I have less investment in what the dart tool does. (cc @jonahwilliams)

@jonahwilliams
Copy link
Contributor

the flutter tool talks to the compiler over stdin/stdout, and what we get over stderr is completely unstructured. Its not possible today for the tool to tell the difference between a warning and an error, so we'd either need to show all of it or none of it.

This came up in the past during the FFI stabilization, the warnings were ultimately removed IIRC.

@Hixie
Copy link
Contributor Author

Hixie commented Jul 1, 2021

Any chance we can prioritize this? It's currently blocking one of the PRs I'm trying to merge over in Flutter land.

cc @leafpetersen

@natebosch
Copy link
Member

Some prior discussion about case 3 (compiling code in order to immediately run it) in #34137

@renatoathaydes
Copy link

renatoathaydes commented Jan 6, 2022

I am seeing this warning as well in a very unexpected place.

I have a package, actors, which is now emitting this warning when a binary depending on it is compiled:

▶ dart compile exe bin/dartle.dart
Info: Compiling with sound null safety
../../../.pub-cache/hosted/pub.dartlang.org/actors-0.8.1/lib/src/isolate/isolate_actor.dart:35:39: Warning: Operand of null-aware operation '??' has type 'String' which excludes null.
  final isolateName = Isolate.current.debugName ?? '';
                                      ^
Generated: /Users/renato/programming/projects/dartle/bin/dartle.exe

This is very weird because I am using Dart 2.15.1 and the actors package uses the environment config sdk: '>=2.15.0 <3.0.0' (same as the package I am currently compiling), which I updated to try to get rid of this warning.

Isolate.current.debugName is certainly nullable, and it appears to have been nullable at least since Dart 2.9.0: https://api.dart.dev/stable/2.9.0/dart-isolate/Isolate/debugName.html (it wasn't nullable up until 2.8.0).

What could be the issue?

EDIT: created a new issue as I think this one is not really very related.

@jacob314
Copy link
Member

+1 on changing the spec so that compilers only emit errors leaving all warnings to the analyzer.
I'm not seeing any value from showing warnings in the compilers as well as the analyzer. Showing warnings from the analyzer works better as we know which packages are in the user's workspace so only show warnings that are actionable for the user to address. Warnings from dependencies are not actionable and we are costing package authors time to try to avoid them on all possible versions of Dart&Flutter that their package might be used with.

An alternate solution would be to teach the compilers about the user's workspace (current package for the simple case, multiple packages for the monorepo case) but that seems like a lot of extra plumbing for minimal benefit given users already see the correct list of warnings in the analyzer.

@leafpetersen
Copy link
Member

I'm not seeing any value from showing warnings in the compilers as well as the analyzer.

As a counterpoint to this, just about every other language I've looked into does provide errors and warnings via the compiler (the notion of a separate analyzer is somewhat idiosyncratic to Dart), so I do wonder whether leaning into the way that Dart does this is the right solution. This is especially true if we want to move towards a world where more of the code is shared between the two. This proposal seems to me to be the most consistent and forward compatible.

@Hixie
Copy link
Contributor Author

Hixie commented May 11, 2022

Dart is IMHO superior to other languages precisely because of this difference. Execution time is a much worse time to give warnings and errors than during development.

@leafpetersen
Copy link
Member

Execution time is a much worse time to give warnings and errors than during development.

Agreed, but Dart is now also an AOT language, in which compilation time and execution time are not always the same. This is why I liked the specific proposal I linked to above: it separates the notion of what tool provides the errors and warnings from when the errors and warnings are provided. If you are developing with the compiler, you should (at least have the option to) get errors and warnings. If you are executing with the compiler/JIT, you should only get errors.

In general, I'm resistant to us building in deep assumptions about how people use our tooling stack without good reason. I see no reason not to support the ability to run a single command to get errors/warnings/compiled output, the way that just about every other tooling stack I've every used supports.

@Hixie
Copy link
Contributor Author

Hixie commented May 11, 2022

I wouldn't object to having a way to get the compiler to also output all the things the analyzer tells us, sure. What I object to is having a different concept of "non-fatal message" from the compiler than what the analyzer outputs. For example, having different lints trigger warnings in the compiler than in the analyzer, having // ignore be ignored by the compiler but not the analyzer, etc. Right now we seem to have two different worlds. IMHO that is a very confusing experience, it's like our tools are having an argument with each other about what the language is.

@leafpetersen
Copy link
Member

I wouldn't object to having a way to get the compiler to also output all the things the analyzer tells us, sure. What I object to is having a different concept of "non-fatal message" from the compiler than what the analyzer outputs. For example, having different lints trigger warnings in the compiler than in the analyzer, having // ignore be ignored by the compiler but not the analyzer, etc. Right now we seem to have two different worlds. IMHO that is a very confusing experience, it's like our tools are having an argument with each other about what the language is.

Yes, I very much support unifying the experience. That said, there are (at least now) some ways in which the compiler is the only way to get some errors and warnings, since the analyzer does not (I believe) support analyzing with different settings for conditional imports.

@jacob314
Copy link
Member

To be clear, I fully support the compilers emitting errors. My concern is with the compiler emitting non-errors, particularly non-errors that are outside of the scope of the user's workspace like it does now.
What we are doing now is similar to if a Java compiler emitted warnings from analyzing .jar files you imported. As everything in Dart is source, it is harder for compilers to know what is your workspace without passing in data that wouldn't typically need to be passed to a compiler.

I agree that the analyzer does not handle analyzing conditional imports correctly today. That is something that should be fixed or the conditional import feature should be improved rather than relying on compilers to warn about conditional import bugs. That said, I do not recall ever catching a conditional import issue with a compiler. When I did bad things that the analyzer couldn't catch, the issues always seemed to show up at runtime.

One other datapoint is that for Bazel workspaces, we are already relying on the analyzer to emit lints and warnings as part of compiles (although we probably end up with mostly redundant warnings from the compilers as well).

@leafpetersen
Copy link
Member

To be clear, I fully support the compilers emitting errors. My concern is with the compiler emitting non-errors, particularly non-errors that are outside of the scope of the user's workspace like it does now.
What we are doing now is similar to if a Java compiler emitted warnings from analyzing .jar files you imported.

No, what we are doing now is similar to gcc emitting warnings if run gcc -Wall. Again, pretty much every compiler toolchain that I am aware of works this way. I dug into this a bit recently in an email thread:

Rust surfaces all errors, warnings, and lints through the compiler.
Swift, I believe surfaces errors and warnings in the compiler.
Java surfaces
Typescript surfaces everything as an error (including things like dead code), but you can disable specific errors.
Javascript defines errors and warnings, I presume all emitted by the compiler?
C, C++ all provide errors and warnings from the compiler, with linters usually shipped as after market accessories.
Java issues errors and warnings through the compiler (dead code is on by default, but many others can be enabled on the command line)
Kotlin seems to provide errors and warnings from the command line compiler.

One other datapoint is that for Bazel workspaces, we are already relying on the analyzer to emit lints and warnings as part of compiles

I would say that this is something we should fix.

@jacob314
Copy link
Member

@leafpetersen and I discussed this a bit more.

Long term we are in agreement that the ideal workflow is an integrated Dart Analyzer + CFE that would enable compilers to can optionally show warnings and lints with compiler output with 100% consistency with what IDEs and dart analyze show. Short term, it makes the most sense to remove all warnings from the compiler output as moving to a consistent model without integrating the Analyzer and CFE would be more trouble than it is worth.

@scheglov
Copy link
Contributor

The analyzer supports conditional imports.
The code below prints b.dart as the imported path, but will switch to a.dart if you set my.flag to false.

import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/file_system/memory_file_system.dart';
import 'package:analyzer/src/dart/analysis/analysis_context_collection.dart';
import 'package:analyzer/src/test_utilities/mock_sdk.dart';

void main() async {
  final resourceProvider = MemoryResourceProvider();

  final sdkRoot = resourceProvider.getFolder('/sdk');
  createMockSdk(
    resourceProvider: resourceProvider,
    root: sdkRoot,
  );

  resourceProvider.newFile('/home/test/lib/a.dart', r'''
void f(int a) {
  a as int;
}
''');

  resourceProvider.newFile('/home/test/lib/b.dart', r'''
void f(int a) {}
''');

  var file = resourceProvider.newFile('/home/test/lib/test.dart', r'''
import 'a.dart'
  if (my.flag) 'b.dart';
''');

  final collection = AnalysisContextCollectionImpl(
    includedPaths: ['/home'],
    resourceProvider: resourceProvider,
    sdkPath: sdkRoot.path,
    declaredVariables: {
      'my.flag': 'true',
    },
  );

  final analysisContext = collection.contextFor(file.path);
  final analysisSession = analysisContext.currentSession;

  final unitResult = await analysisSession.getResolvedUnit(file.path);
  unitResult as ResolvedUnitResult;

  print(
    unitResult.libraryElement.importedLibraries
        .map((e) => e.source.fullName)
        .toList(),
  );
}

@leafpetersen
Copy link
Member

The analyzer supports conditional imports.
The code below prints b.dart as the imported path, but will switch to a.dart if you set my.flag to false.

I stand corrected. How do we surface this to users?

@leafpetersen
Copy link
Member

Short term, it makes the most sense to remove all warnings from the compiler output as moving to a consistent model without integrating the Analyzer and CFE would be more trouble than it is worth.

I would phrase this as "more effort than we can afford right now", but otherwise LGTM.

@scheglov
Copy link
Contributor

Well, this is a right question. Because currently we don't.

IIRC we had something like -Dfoo.bar=xyz in dartanalyzer (which we want to remove). Now for conditional imports, but for constant evaluation. But apparently it was removed. Theoretically it could be re-wired into dart analyze and analysis server (which is the implementation behind dart analyze).

For IDEs, we can either devise a way to declare variables in analysis_options.yaml (this is somewhat strange, because this is a file in Git, and other developers might want different values), or have a way for users to declare these variables in some ephemeral way in IDE (e.g. the same -Dfoo.bar=xyz in analysis server options).

Anyway, I just wanted to make it more specific - the analyzer does support conditional imports, but user facing tools don't. Which might be a slightly simpler problem to fix implementation wise, although might cause additional arguments about UX :-)

@leafpetersen
Copy link
Member

Anyway, I just wanted to make it more specific - the analyzer does support conditional imports, but user facing tools don't. Which might be a slightly simpler problem to fix implementation wise, although might cause additional arguments about UX :-)

Gotcha, thanks.

@Hixie
Copy link
Contributor Author

Hixie commented May 12, 2022

Again, pretty much every compiler toolchain that I am aware of works this way.

Ideally we would be better than the other compiler toolchains, not as bad as them. :-)

@leafpetersen
Copy link
Member

Again, pretty much every compiler toolchain that I am aware of works this way.

Ideally we would be better than the other compiler toolchains, not as bad as them. :-)

But certainly not worse. In other words, I strongly believe that what the other toolchains do here is the correct thing to do, and that what we do is not, and I would like to see evidence (or at least an argument) to convince me otherwise.

@Hixie
Copy link
Contributor Author

Hixie commented Jun 3, 2022

I don't object to warnings in general, if they can be silenced (using the same mechanisms as the analyzer, currently // ignore, .analysis_options.yml, and so on). I do think it is very confusing that the compiler and the analyzer disagree about what warnings should be shown.

Personally I don't ever want these warnings; when I say flutter run I just want it to run because I'll be using hot reload and the analyzer to get feedback (do these warnings show up with every hot reload?), and when I use dart run I am usually doing it because I'm using my app and redirecting stderr and stdout and don't want the compiler contributing its own opinions into my outputs.

@jacob314
Copy link
Member

jacob314 commented Jun 3, 2022

@mit-mit If we hide warnings from packages that the developer depends on and respect // ignore and .analysis_options.yaml in the CFE then I'm fine with leaving the warnings on.

@johnniwinther
Copy link
Member

Supporting // ignore in the CFE is major task that might also hit performance. Currently the analyzer parses all comments whereas the CFE skips them at the earliest state in the compilation, and the semantics of the ignore comments (the used names) is directly tied to the analyzers messaging system, which is not shared with the CFE.

@jacob314
Copy link
Member

jacob314 commented Jun 7, 2022

Given that supporting // ignore in the CFE is a major task that might hit performance, I think the current solution of hiding warnings by default for flutter run||build is the best solution.
We should revisit in the future if there is a performant way to run the linter as part of the CFE.

@mit-mit
Copy link
Member

mit-mit commented Jun 8, 2022

Can you point to any analysis of how common using // ignore for warnings is? My gut tells me that it's pretty uncommon, but I have no actual data behind that.

@Hixie
Copy link
Contributor Author

Hixie commented Jun 8, 2022

There's 614 // ignores in the flutter/flutter codebase alone, FWIW.

@jacob314
Copy link
Member

jacob314 commented Jun 9, 2022

There are about 150 // ignore in flutter/devtools. I don't see anything fundamentally less ignore-able about most of the existing Flutter warnings than many of the lints we enable by default. There is a selection bias today as ignoring warnings doesn't really seem like it works as you can still see the warnings in your output when you flutter run.
Here is an example of ignoring invalid_null_aware_operator in a fairly popular dart command line tool:
https://github.com/trevorwang/retrofit.dart/blob/master/generator/lib/src/generator.dart#L799

In g3 there is at least one code generator that generates code withignore_for_file: invalid_null_aware_operator
That seems like a fairly reasonable thing for a code generator to do as there is no real harm in adding an extra ? if it makes it easier to write the code generator.

@natebosch
Copy link
Member

natebosch commented Jun 9, 2022

Here is an example of ignoring invalid_null_aware_operator in a fairly popular dart command line tool: https://github.com/trevorwang/retrofit.dart/blob/master/generator/lib/src/generator.dart#L799

This is not a valid ignore. It may have been valid in older SDK versions? It's maybe an artifact of an out of order migration?

@mit-mit
Copy link
Member

mit-mit commented Jun 9, 2022

I'm aware that ignoring lints is pretty common. But given the the topic here is hiding of CFE (common front end) warnings, I think the relevant data point is about hiding those (or analyzer warnings similar to those)?

@jakemac53
Copy link
Contributor

Fwiw, I think you could make a pretty solid argument that the invalid_null_aware_operator, in the situation described, is emitting a false positive. The ? is in fact required, because the code is explicitly trying to handle multiple versions of the dependency, and it can safely do so by including a ?. I don't believe it can avoid this without taking into account package version constraints, which the compiler probably should not do, and for that reason it should be a lint and not a warning, and removed from the compiler.

The same probably goes for most if not all warnings defined in the spec.

Yes, the language tries to pretend version constraints and pub itself do not exist. But, the fact of the matter is they do exist, and this warning is actually actively encouraging you to make potentially unsafe changes, because it doesn't understand those things.

@bwilkerson
Copy link
Member

I don't believe it can avoid this without taking into account package version constraints, which the compiler probably should not do ...

I agree that the compiler shouldn't look at package version constraints. But it's less clear to me that we shouldn't do that in the analyzer. The analyzer is intentionally not just a compiler, so if it would be more helpful to our users for it to consider package version constraints, I'm not sure we should take that option off the table without first doing a cost/benefit analysis. (Analyzer already does a fair bit of analysis of the pubspec.yaml file.)

@jakemac53
Copy link
Contributor

But it's less clear to me that we shouldn't do that in the analyzer.

Right, conceptually I think it would be nice if the analyzer could take your package version constraints into account. In practice I think it might not be feasible, but that is a different question. I could imagine a lot of benefits though.

@Hixie
Copy link
Contributor Author

Hixie commented Jun 9, 2022

FWIW in a bunch of Flutter code to avoid the unignorable compiler warning log spam when writing code that deals with APIs that are nullable in old versions and non-nullable in new ones we've explicitly used hacky functions that happen to not cause any warnings today, like this:

/// This allows a value of type T or T? to be treated as a value of type T?.
///
/// We use this so that APIs that have become non-nullable can still be used
/// with `!` and `?` on the stable branch.
T? _ambiguate<T>(T? value) => value;

Interestingly, this pattern is starting to seep into the ecosystem:

https://www.google.com/search?q=flutter+%22ambiguate%22+site%3Apub.dev

@jakemac53
Copy link
Contributor

It's the new unawaited! lol

@leafpetersen
Copy link
Member

FWIW in a bunch of Flutter code to avoid the unignorable compiler warning log spam when writing code that deals with APIs that are nullable in old versions and non-nullable in new ones we've explicitly used hacky functions that happen to not cause any warnings today, like this:

Without wading into this again, I would strongly caution against driving long term strategic choices in our tooling off of short term issues like this. Mixed mode code won't be a thing within a year or so. There may (or may not) be good reasons for doing this long term, but short term pain around the migration is not a good motivation for driving the long term UX of our product. I would strongly suggest staying focused on the long term UX of the language that we want to see for our users.

@Hixie
Copy link
Contributor Author

Hixie commented Jun 15, 2022

This is not a mixed mode issue, it's an issue of an API that changed from nullable to non-nullable, which is not going away. I agree that we should focus on long term issues; the above is an example of the kind of thing people are doing because of (unignorable bogus) warnings today, and I see no reason to believe that's a short-term issue.

@lrhn
Copy link
Member

lrhn commented Jun 15, 2022

That is actually a very interesting situation.

Changing a function from returning Foo? to returning Foo is a safe and non-breaking change (modulo the usual caveats about people implementing interfaces, but say it's a static function).

It's non-breaking because everything you can do on a Foo?, you can also do on a Foo (and more!), that's what object oriented subtyping is all about.

Some of the things you can do on Foo? are unnecessary on Foo. That's because we have operations on Foo? that are not just methods. If ?? was a method on Foo?, then it was also a method on Foo, and nobody would complain about calling it. Language-level operators are not virtual methods, the compiler knows and understands what they do (heck, the language specification knows), which is why we know when using them is unnecessary.
(We could also have an annotation @NullableOnly used on extensions on nullable types, which introduces warnings if you use it on a non-nullable type, because it's unnecessary there. That would make sense too.)

That's where the warnings come in. Warnings are not errors, so the change is still non-breaking, even if it makes existing code get warnings for doing unnecessary things, and out tools are able to recognize those null-related unnecessary things.
But analysis only ever takes one version of the code into consideration.

This is code that wants to be compatible with multiple distinct versions of the same API, and it does so by doing things that are unnecessary for some of the versions.
That gives rise to warnings when analyzed only against the other versions.
The warning is technically correct (best correct!) in the context where analysis runs, but its not absolutely correct in the global context.

And therefore it makes sense to be able to silence the warning. There is no workaround, no way to rewrite and not get the warning while still satisfying the original goal.

That's why all warnings must be silenceable. There can be reasons for keeping the warning-triggering code in the program, reasons why the analysis leading to the warning is wrong, because it just doesn't have all the facts.
We know that's the case, otherwise it could have been an error.

So, either make compilers respect // ignore (which is a big effort), or make them be able to not emit warnings at all. (Or, heck, let them accept a file of "ignore warnings" declarations which say which files and line/column ranges to not show warnings for, or maybe even specific warnings. Then we can create a tool to generate such a file from // ignore: comments. Or something else, options are plenty.)

(If we want a general helper instead of _ambiguate, maybe:

extension MakeNullable<T extends Object> on T? {
  T? get nullable => this;
}

)

@gmurray81
Copy link

Here's my two cents on this. I'm working on transpiling another language to Dart to use in a package for flutter.
If the compiler emits non-ignorable warnings for things like redundant null assertion operators then it is literally asking for my transpilation's flow analysis to be at least as complex as the flow analysis that is causing this warning to be emitted. When, alternatively, if I can assert that I'm using the operators appropriately enough (if redundantly) in the transpilation output, being able to just ignore the error is far far preferable to taking on unbounded amounts of extra complexity in tracking with Dart's code flow analysis.

If I could just ignore the error when I build that would be one thing, but because the end user is the one compiling all the packages, I really need to prevent them from seeing a warning if I have asserted that it is ok.

@bwilkerson
Copy link
Member

@gmurray81

I can think of a couple of solutions for you situation. The first is to generate an ignore_for_file comment at the top of every file. That will cause the diagnostics to not be reported to your users.

The second, and in my mind better, is to run dart fix over the generated files. It has the ability to fix that particular diagnostic, which means that the code would be better without you needing to re-implement flow analysis (which I agree you want to avoid doing for several reasons).

@leafpetersen
Copy link
Member

The first is to generate an ignore_for_file comment at the top of every file. That will cause the diagnostics to not be reported to your users.

I don't think the compilers respect ignore comments (at least not yet). The dart fix suggestion seems like a good workaround. Just in the spirit of suggesting expedient workarounds (not to deny the validity of your use case, thanks for the input!), another option might be to always first upcast to the nullable version of the type before emitting the ?.. That is, emit (e as T?)?.foo instead of e?.foo. That is, assuming you reliably know the type of e.

@gmurray81
Copy link

yes, I have been unable to find a way to ignore the warning with ignore_for_file. I'm of the mind that no warning should ever be unignoreable unless its really an error. And if an automated "fixer" can find a warning and absolutely reliably auto correct it, then it should definitely be a warning, and, I believe, handled by the analyzer rather than the compiler.

The dart fix suggestion is interesting to me. However, I'd have a few questions about that approach:

  1. is it possible to run this targetting a specific rule in order to reduce the scope of the churn.
  2. is running dart fix gauranteed not to impact the behavior of the modified logic? Or it is assumed that a human is going to review the modifications for validity.

If a human is supposed to confirm validity of the mutations then this probably isn't a valid approach to work around the issue.

Isn't an unnecessary shift to a nullable context going to generate its own unignorable warning maybe?

All in all, this should simply be an ignorable warning. Generated/Transpiled code is a valid use case that should be considered in the tooling.

@gmurray81
Copy link

as an addendum, a language that seems to be trying to eschew reflection in a lot of cases should make special affordances to not interfere with the ease of generating logic without compulsory warnings.

@leafpetersen
Copy link
Member

2. is running dart fix gauranteed not to impact the behavior of the modified logic?

I believe this is the case.

Isn't an unnecessary shift to a nullable context going to generate its own unignorable warning maybe?

This will generate an analyzer hint (which can be ignored, but which will probably not be visible to end users anyway since it is presumably in a different package). It will generate no compiler diagnostic.

All in all, this should simply be an ignorable warning. Generated/Transpiled code is a valid use case that should be considered in the tooling.

Acknowledged.

@lrhn
Copy link
Member

lrhn commented Dec 22, 2022

The argument against up-casting is that the compiler can then not (or not as easily) choose to just ignore the null-assert/null-aware check. An e?.foo where e is non-nullable should be trivially equivalent to just e.foo, but (e as T?)?.foo needs the compiler to recognize that (e as T?)?.foo is just e.foo as well. Which it is, even for extension methods, but not as trivially.
It works, but by doing two unnecessary operations instead of one, to switch one warning to another hint.
It'd be preferable to not have to do anything.

@Hixie
Copy link
Contributor Author

Hixie commented Mar 31, 2023

The issue linked above (VeryGoodOpenSource/dart_frog#460) is an interesting example of how this is causing more confusion than help in the wider community, IMHO.

@johnniwinther
Copy link
Member

cc @stereotype441

@munificent
Copy link
Member

We have changed the language specification so that tools are no longer required to report all specified warnings. More context is here.

This shouldn't be interpreted to mean that the language team prefers that any particular tool does not report warnings. We are just removing a constraint on the discussion. If, say, CFE decides they think it's best to stop reporting warnings, now they can do that without being in violation of the spec. But if CFE wants to continue reporting some or all of the warnings, they are free to do that as well. It's a product/UX decision now and not a specification/correctness issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool.
Projects
None yet
Development

No branches or pull requests