DDC should not apply additional restrictions from analysis_options.yaml to my package

leafpetersen commented 7 years ago

IDE and command line analysis only reports errors and warnings from the current project. Since DDC must build all dependencies, it also reports any errors and warnings from imported packages. Opt in restrictions (e.g. --no-implicit-casts) from a .analysis_options file currently will fail the build if packages do not conform to them. This is probably not the right behavior:

https://github.com/dart-lang/sdk/issues/29654

kevmoo commented 7 years ago

Or analysis_options.yaml 😄

nshahan commented 7 years ago

I just ran into this issue when building https://github.com/dart-lang/angular_components_example with DDC via pub serve --web-compiler=dartdevc.

jmesserly commented 7 years ago

I believe this is an issue with pub build, not DDC.

DDC will issue errors for every source file it compiles. If there's something "outside my current project" it needs to be compiled separately, and it must be compiled without analysis flags that are intended for my project.

nshahan commented 7 years ago

@jakemac53 Any thoughts about fixing this in pub?

natebosch commented 7 years ago

There is a DDC issue - the problem is that analysis_options.yaml can make DDC treat some things as errors that it shouldn't. If DDC can build a project that doesn't pass analysis with --no-implicit-dynamic it shouldn't suddenly start failing when I add that option to my analysis options - only analysis should fail.

Current vs external packages doesn't really matter - DDC should never change it's behavior based on analysis options.

natebosch commented 7 years ago

We could maybe forcibly solve this in pub if DDC exposes a --options= flag the the dartanalyzer binary does... but I think DDC should just always configure it's AnalysisDriver to use an empty set of options.

jmesserly commented 7 years ago

I think DDC exposes all options flags dartanalyzer does? At least we tried to unify all flags & there's shared options code.

If DDC can build a project that doesn't pass analysis with --no-implicit-dynamic it shouldn't suddenly start failing when I add that option to my analysis options - only analysis should fail.

RE .analysis_options, current invariant is that dartdevc produces the same errors as dartanalyzer does. This is intentional, and that's why no-implicit-dynamic is respected in your .analysis_options (for the current project), similar to no-implicit-casts. It is true that both of those options would be safe to ignore at compile time, but in general that is not true for options that affect strong mode's checking.

leafpetersen commented 7 years ago

I'm hesitant to make empty flags always the behavior. There are workflows in which analysis and compilation are separate, but I think there are many people who are used to the workflow which is standard for other compilers: you run the compiler both to get errors and warnings, and to get the compiled output.

natebosch commented 7 years ago

Attaching analysis to compilation precludes enforcing anything in your code that won't also hold for generated code. For instance if the angular compiler doesn't pass --no-implicit-dynamic it means I also can't check that with my own code.

leafpetersen commented 7 years ago

Right, hence this bug. I would like it if users could turn on --no-implicit-dynamic for their package only, and not have it transitively apply to other packages.

natebosch commented 7 years ago

The generated code is in their package. We'd need to be able to put generated code in a separate module from code written by the user - and that precludes any strongly connected import graphs into generated code.

leafpetersen commented 7 years ago

How is that specific to DDC then? If the code is in their package, then both the analyzer and DDC will complain, no? So they just flat out can't turn it on. The use case I want to enable is where the bad code is not in their package, but they are currently blocked from turning it on because they will get warnings about code that is elsewhere, not in their package.

natebosch commented 7 years ago

Synced offline: I think one of the big points of contention is that analysis_options.yaml is used to configure things like what we see in the IDE and whether Travis blocks a submit- and the goals there are different from what we expect during a compile.

In the long term we most likely want some way to distinguish between configuring how DDC does analysis and how the IDE/Travis/dartanalyzer does analysis.

In the short term we'll fix the bug with using the local analysis_options for external packages and just won't allow using analysis options that don't pass with generated code.

bwilkerson commented 7 years ago

... analysis_options.yaml is used to configure things like what we see in the IDE ...

I think that's too narrow a view. It's called "analysis" options for historic reasons, but many of the options ought to apply to all our tools (and ought to be supported by FE).

natebosch commented 7 years ago

What I mean is that our users expect to be able to have different configuration for analysis in different places and that's not possible today.

We still have some open questions about behavior today:

How does DDC act when there are excludes? If DDC could treat an excluded file as "Go back to defaults" that might be enough for us to work around the generated file case.
How does DDC act when an 'error' is downgraded to a 'warning' or 'hint' but breaks behavior of the compiler?

Here's a scenario I think we need to be able to support:

I want to never push code which fails some hint - say I never want to push code that has unused variables. When I run dartanalyzer on Travis it should have a failing exit if it sees an unused variable.
In my IDE I should see that this hint is critical, I'd prefer for it to highlight as an error.
During my development I would prefer if DDC does not fail due to unused variables.
At any time DDC must not fail due to unused variables in angular generated code.

bwilkerson commented 7 years ago

How does DDC act when there are excludes?

The behavior most consistent with analyzer behavior would be for it to not compile excluded files unless they are referenced from non-excluded files. (That's assuming that ddc has a "compile this directory" mode, otherwise I guess I don't understand the question.)

How does DDC act when an 'error' is downgraded to a 'warning' or 'hint' but breaks behavior of the compiler?

During my development I would prefer if DDC does not fail due to unused variables.

The displayed severity of a diagnostic can be different than the "impact" (for lack of a better word) of the diagnostic. I'd interpret the options as controlling the displayed severity and having nothing to do with the impact.

jmesserly commented 7 years ago

@natebosch a good mental model for DDC is:

run dartanalyzer build mode (in the future: run new front end)
run the code generator

errors/warnings/hints are produced by step 1. DDC simply prints them out.

How does DDC act when there are excludes? If DDC could treat an excluded file as "Go back to defaults" that might be enough for us to work around the generated file case.

To my knowledge, analyzer build mode (thus DDC) do not consider "excludes" because it relies on an explicit list of sources (like DDC). The excludes are about filtering what IDEs show.

How does DDC act when an 'error' is downgraded to a 'warning' or 'hint' but breaks behavior of the compiler?

what does "breaks behavior of the compiler" mean? DDC is fine compiling if there are warnings/hints.

DDC/analyzer build mode are just compilers. Build systems have lots of ways to configure compilers and filter output from compilers. e.g. feel free to filter errors.

(Tho I'd advise against treating generated code specially; it'd be much better to clean up the code generators.)

natebosch commented 7 years ago

How does DDC act when an 'error' is downgraded to a 'warning' or 'hint' but breaks behavior of the compiler?

what does "breaks behavior of the compiler" mean? DDC is fine compiling if there are warnings/hints.

Let's say I write this code:

int x = 'foo';

And this analysis_options.yaml:

analyzer:
  errors:
    invalid_assignment: ignore

and then try to compile this with DDC. What would happen? Would I get undefined behavior at runtime?

natebosch commented 7 years ago

The displayed severity of a diagnostic can be different than the "impact" (for lack of a better word) of the diagnostic. I'd interpret the options as controlling the displayed severity and having nothing to do with the impact.

Today in order to get the 'displayed severity' I want I configure it in my analysis_options.yaml - and then it has the 'impact' of breaking my DDC build - that is what I hope to avoid. Also to allow the 'displayed severity' to align with the 'impact' of blocking submits in my CI server (preferably without repeating my config multiple places). Also I'd want the 'displayed severity' to not be tied to my IDE but to be shared across all devs on my project who see output from the analyzer.

natebosch commented 7 years ago

(Tho I'd advise against treating generated code specially; it'd be much better to clean up the code generators.)

Generated code will always be special in some ways - I don't need to maintain it. If I consider unused variables to be a maintenance/readability issue I might never care that generated code has this problem. For runtime code I only care about errors/warnings/hints if they impact performance or behavior - but I today depend on the dartanalyzer (and analysis_options config) to help me keep my manually written code maintainable.

bwilkerson commented 7 years ago

Today in order to get the 'displayed severity' I want I configure it in my analysis_options.yaml - and then it has the 'impact' of breaking my DDC build - that is what I hope to avoid.

(italics added) I don't understand that statement. If the analysis options file only controls how the diagnostic is displayed, then it cannot break your ddc build.

Also to allow the 'displayed severity' to align with the 'impact' of blocking submits in my CI server (preferably without repeating my config multiple places).

The impact of an error depends on the tool being impacted. The same error could impact a CI server by preventing a commit while impacting ddc by causing a throw to be inserted at the site of the error.

Also I'd want the 'displayed severity' to not be tied to my IDE but to be shared across all devs on my project who see output from the analyzer.

Hence the analysis options file, because it can be shared across tools.

matanlurey commented 7 years ago

If the analysis options file only controls how the diagnostic is displayed, then it cannot break your ddc build.

Hence this bug...

natebosch commented 7 years ago

(italics added) I don't understand that statement. If the analysis options file only controls how the diagnostic is displayed, then it cannot break your ddc build.

That's what this issue was filed for - today anlaysis_options.yaml does break the DDC build if I upgrade something to an error.

natebosch commented 7 years ago

For the record if I manually downgrade invalid_assignment: ignore and then compile this snippet:

void main() {
  int x = 'foo';
  print(x + 1);
}

The result is a warning, the build does not fail. And the code produced is:

  foo.main = function() {
    let x = 'foo';
    core.print(x + 1);
  };

Prints "foo1" but we could probably imagine other ways to get behavior we don't want. I think there are probably certain errors that DDC shouldn't let you downgrade.

jmesserly commented 7 years ago

DDC already has --unsafe-force-compile to ignore all errors. Ignoring errors will lead to undefined behavior.

natebosch commented 7 years ago

Ignoring errors will lead to undefined behavior.

Sure - and if the file were called compiler_options.yaml I'd expect that downgrading an error gives me this risk 😄

leafpetersen commented 7 years ago

I'd definitely prefer that we not leave open a gate for people to pick and choose their undefined behaviors... :) Not top priority, but whether we keep using analysis_options.yaml or a new file, we should probably not honor downgrading errors to warnings there.

jmesserly commented 7 years ago

we should also check if we picked up any command line options from Analyzer CLI that allow errors to be ignored

natebosch commented 7 years ago

Filed an issue to track the pub behavior and removing the other avenues towards downgrading errors in DDC.

I think this issue is still useful to track whether we want a compiler_options.yaml or some other way to drive different behavior between DDC and other uses of analyzer.

matanlurey commented 7 years ago

How is this different from pubspec.yaml's compiler: option?

compiler:
  web:
    ...

anders-sandholm commented 7 years ago

@jmesserly, sorry for adding area-dev-compiler back in. It seems to be the most suitable "area-" label available. Without an area- label issues will keep showing up in the "needs to be triaged" list. Happy to change to some other more relevant "area" label. E.g., re-introducing "area-pub"?

jmesserly commented 7 years ago

oh yeah, totally okay! tho maybe we need an area-build or something like that :)

davidmorgan commented 7 years ago

My experience today: enabled DDC for an open source project, it refused to compile with errors in package:quiver and package:collection. Huh? Aren't these ready for DDC? Maybe my SDK is out of date ... upgrade to 1.24.2 ... same problem ... investigate the errors in package:quiver, realize they look like "implicit-dynamic" errors, try turning it off, discover that it now compiles.

This seems like a pretty major roadblock in asking people to switch from Dartium to DDC. A note over at https://webdev.dartlang.org/tools/dartdevc would have worked. (Recommending what? Downgrading analysis for my project because package:quiver has problems seems ... wrong).

Thanks!

natebosch commented 7 years ago

@davidmorgan you want to track https://github.com/dart-lang/pub/issues/1684 for that issue.

I updated the title to reflect the new state of understanding for this issue. If anyone things we should branch of the conversation around whether DDC should ignore analysis_options.yaml for the local package we can cut a new issue and close this one.

davidmorgan commented 7 years ago

Thanks.

jmesserly commented 7 years ago

I updated the title to reflect the new state of understanding for this issue. If anyone things we should branch of the conversation around whether DDC should ignore analysis_options.yaml for the local package we can cut a new issue and close this one.

sounds good to me. I've opened this bug: https://github.com/dart-lang/sdk/issues/30422 & will link this thread for context.

natebosch commented 7 years ago

Filed https://github.com/dart-lang/sdk/issues/30827 to track allowing different configuration per tool.

kevmoo commented 7 years ago

I thought @jakemac53 fixed this?

matanlurey commented 7 years ago

I have not tried dev. Will try and close this issue

On Wed, Sep 20, 2017, 8:51 AM Kevin Moore notifications@github.com wrote:

I thought @jakemac53 https://github.com/jakemac53 fixed this?

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

rwrozelle commented 6 years ago

Shouldn't the pubspec.yaml have a 2.00 dev sdk?

  sdk: 2.0.0-dev.5.0

Or at least a note in the README? since the local pubspec.yaml will override the dependencies.

See https://pub.dartlang.org/packages/angular/versions/5.0.0-alpha#pub-pkg-tab-changelog

dart-lang / sdk

DDC should not apply additional restrictions from analysis_options.yaml to my package #29713