[parts-with-imports] Conditional part directives and the analyzer about language

[lrhn]

each conditional library will be analyzed independently as well,

That can fail if the libraries and class have dependency cycles.
If, say, a library defines a factory class, conditionally imports and exports an implementation class, and the implementation class refers back to the factory class, then at most over of the implementations will see itself as the type returned by the factory class.
The rest will have your errors.

With parts, it's just more likely that there are cyclic dependencies, even if each part could be analyzed independently.

Would it be worth defining some configuration that a part assumes,
Like

part of "parent.dart" if (dart.library.io);

I guess it makes no difference, the condition can be trivially inferred from the part declaration that refers back to the part file. It's the ability to hold multiple versions of the same library that's needed.

[bwilkerson]

each conditional library will be analyzed independently as well,

That can fail if the libraries and class have dependency cycles.
...
With parts, it's just more likely that there are cyclic dependencies, even if each part could be analyzed independently.

I think that cyclic dependencies can cause analysis to fail in the sense that the analyzer could produce false positives. But if we don't do something more for parts we could have false negatives (by not analyzing whole subtrees).

I think users will be more (negatively) impacted by not seeing errors in their code until they build for a given platform than by seeing false positives. That doesn't mean that I'm convinced that we have to analyze every part, I just wanted to make sure everyone is aware of the situation and that we've made a deliberate decision.

It's the ability to hold multiple versions of the same library that's needed.

Do you mean "needed" in the sense of "what would be required to completely solve the problem" or in the sense of "is a requirement we're placing on the analyzer"?

[scheglov]

FYI, in Rust code that is excluded by #[cfg()] is not analyzed semantically.
It is parsed, and parse errors are reported.
But no resolution.

Note that on this screenshot windows code is greyed out.

[jakemac53]

How does rust manage setting up the configuration such that this can happen? IIRC the main issue the analyzer has had is that it doesn't have any way of telling it what configuration to analyze with? Can we learn something from them?

[scheglov]

tl;rd there are separate ways / levels for rustc, cargo, and IDE.

Documentation is here.

Some of configuration options are compiler-set, such as target_os.
Other can by declared by the user.
I think we can provide them with --cfg to the rustc - the Rust compiler.
I don't know if this is the right level, my current understanding is that going through crate is preferred.

For crate, to run with crate run (which will invoke rustc for us), we can work with features.

If I run with cargo run, nothing is printed.
If I run with cargo run --features print_enabled, it will print.
Or if I enable the feature by default with default = ["print_enabled"] in Cargo.yaml.

Also, notice that there is the gray mark in RustRover.
If I toggle it, this changes whether RustRover compiles the configured code.
I was not able to find where it keep this flag, maybe somewhere outside the project directory.
This does not affect what cargo does, only analysis in the IDE.
And because this is the same flag, it enables all pieces of code that use it.

[scheglov]

Currently we analyze with default URIs, which works to some extent.
But we don't analyze and report any diagnostics for non-default configurations.
For example a non-default imported library could miss one of the invoked functions.
And the user will know about this only from the compiler.

I can see several possible solutions.

1. Say that this is fine.
2. Support a way to provide configuration flags in analysis_options.yaml or similar file.
3. Like (2), but let the user to describe the set of values for each flag, and the analyzer should analyze each affected file in all combinations.

For conditional parts the situation is wore because a part cannot be analyzer outside of its library (in contrast to a conditionally imported library). So, we will not able to analyze these parts at all - no diagnostics, no semantics functions like navigation or code completion. So, (1) is not a good answer anymore.

I suspect that (3) theoretically could be implemented, but would be fairly complicated.

[jakemac53]

(3) would also be expensive right? Would it be a whole new analysis context for each, which would separately analyze the whole project?

[scheglov]

Actually, you are right, we could do (3) in a much less complicated way, just for separate analysis contexts.

Computationally it will be as expensive as necessary to reflect the potential effect of each flag.

If some libraries don't have anything that depends on a flag in their transitive dependencies, we will reuse from summaries their element model, and diagnostics. There will be some CPU / heap cost of creating FileState models and computing hash based cache keys.

And if something is affected, there is no choice - we have to compute new results.

[bwilkerson]

(3) would also be expensive right?

Yes. It would be expensive in terms of performance because we'd have to repeat the analysis once for every combination of flags that we needed to analyze against. And it would be expensive in terms of memory for the same reason, though we might be able to share some flag-independent state across contexts.

And if the exported namespace of the library is different under different combinations of flags, then we'd theoretically need to analyze every library that imports it against each version of the namespace.

And then, of course, there's the added complexity for the user: such as needing to figure out which combination of flags caused a given diagnostic to be reported.

The question is: which situation would be best for users, and can we afford to implement it.

I do have to ask, though, whether there might be a fourth option. I think we could analyze each of the only-conditionally-included parts against the default namespace of the library (and for conformance with the default part), and not add any declared names to the default namespace. That wouldn't be perfect. There'd be both false positives and false negatives in some situations, but it might be less expensive than a complete analysis while providing better feedback for the user most of the time.

[scheglov]

We can do any computation infinitely faster, if the correctness of the result does not matter :-)

I think in (4) it would be annoying to have either false positive, or false negative diagnostics in non-default conditional parts. And similarly, it would be inconvenient to be not able to use correct code completion, navigation, and other semantic services.

Just give me full fidelity in one combination, i.e. (2). Want to focus on Windows (or dart:io, whatever) portion - say so; then switch to another configuration - validate it, make changes, etc. Not all configurations at once, but all features in one (non necessary default) configuration.

I agree about complexity of (3) for the user. We could try to mitigate this by adding the combinations of flags that produced a diagnostic, when not every combination produces them.

(3) is definitely not free, and will use more CPU and heap. But it might be up to the user to decide what he wants - manually iterate over combinations of flags, or pay the price and get it all automatically.

Although... even if we display a diagnostic, e.g. the type of the argument is not assignable to the type of the formal parameter, if the navigation does not go to the location of the invoked method, it would be not convenient.

[bwilkerson]

I can't say with any certainty how often there would be false positives or negatives in (4). So while I agree that false positives and negatives would be annoying, I don't know whether that would be a problem in practice.

I have two concerns with (2). The first is discoverability. I don't think it would be obvious to most users that they can set the combination of flags in a file and have it control analysis. The second is usability. Even knowing that the control exists, I have to wonder whether users would remember to change the configuration when they need to do so. We might need to prompt the user, for example by displaying a message saying something like "You are editing a file that is not currently being analyzed because of the configuration selected in the file. In order for this file to be analyzed you need to set to true.". But that too could become annoying.

Perhaps we need to get some feedback from users via a survey question or a user study?

[scheglov]

I think it is trivial to construct a false positive case for (4).

part 'foo.dart'
  if (dart.library.html) 'foo_html.dart';

void f() {
  do_foo();
}

In foo.dart

void do_foo() {}

In foo_html.dart

class FooHtml {
  void do_foo() {}
}

void do_foo() {
  FooHtml().do_foo();
}

If we don't analyze foo_html.dart with the right scope, it will not able to resolve FooHtml (because if the part is not included into the library, its top-level declarations are not included), and report a diagnostic.

And the false negative is this structure, but when FooHtml is removed, and we don't report diagnostics for non-default URI parts to avoid the previous false positive.

Using a header like we have for pubspec.yaml files in IntelliJ might be a good way to solve both issues: the user will learn that the file is not analyzed, and it can have a reference to the documentation that describes how to set values for flags.

[bwilkerson]

If we don't analyze foo_html.dart with the right scope, it will not able to resolve FooHtml (because if the part is not included into the library, its top-level declarations are not included), and report a diagnostic.

I agree that using the right scope during resolution is critical. I guess I just assumed that the scope we'd use for a given part would include the declarations in that part (and any other part in the subtree rooted at that part).

More generally, I have to wonder whether we couldn't create a scope for the analysis of these conditional parts that would minimize both false positives and false negatives.

[scheglov]

Now imagine that FooHtml is exported from bar.dart

export 'bar.dart'
  if (dart.library.html) 'bar_html.dart';

and imported into the library:

import 'bar.dart';

part 'foo.dart'
  if (dart.library.html) 'foo_html.dart';

void f() {
  do_foo();
}

and we still have foo_html.dart

void do_foo() {
  FooHtml().do_foo();
}

If we did not use the same dart.library.html flag during linking bar.dart, we will get false positive.

[bwilkerson]

True. I already conceded that (4) would have both false positives and false negatives. The question is whether the number of times we erroneously analyze code is small enough in practice to make this a viable alternative.

I am still concerned by the implications of (2) as mentioned above, as well as a third issue I've thought of, which is the danger that someone on a team might accidentally check in the file that specifies the configuration so that other team members' analysis might change without them realizing it. The idea of requiring users to be aware of the current configuration seems problematic to me.

If

• users could specify possible configurations in a file,
• we could display the current configuration in an easy to see place, and
• user's could dynamically change the current configuration by selecting it in a drop-down

then I'd be more comfortable with it because it would be more visible and easier to use. But I don't think any of those are actual options.

[scheglov]

I think this is a bad exchange: correctness vs. awareness.

If the developer knows what he is doing, with (2) he can always get correct analysis.
With (4), if there is a false positive, he is out of luck, no solution.

The current active configuration is not any different than any other configuration in analysis_options.yaml (what if someone ignored a lint?), or anything in Dart code. This is why there is code review.

Why don't we display easily all active lints in our IDEs? :-)

There is a precedent in Rust, there are many crates that have features that can be optionally enabled. So far I have not seen any warnings in books that I read that this is bad and should be avoided. And in RustRover code that is behind a not enabled condition is not analyzed semantically. Developers use Crate.toml to configure enabled features just fine.

[bwilkerson]

I think this is a bad exchange: correctness vs. awareness.

In general, I agree.

The question in my mind is the percentage of users that will see an incorrect analysis in each case.

For (4), all users working on non-idiomatic code will see incorrect analysis. But if the number of users working on non-idiomatic code is small, then so will be the number of users impacted by this choice.

For (2), any user that doesn't understand how our tool deals with configurations or how to change them will see incorrect analysis. Users don't read documentation, and the ability to set a configuration would be new and hard to discover, so I'm concerned that most users won't understand it. The number of impacted users would be limited to those working in code bases with conditional parts, but I don't have a good guess as to how many that will be.

Also, the failure mode for (4) is generally going to be obvious. The failure mode for (2) is that there will be that there's no indication that some of the code wasn't analyzed.

The current active configuration is not any different than any other configuration in analysis_options.yaml ...

Yes, it is. The active configuration determines which code is analyzed at the moment; the other configuration options (other that the exclude list) configures how the code will be analyzed, but not whether it will be analyzed. (And the exclude list is a 'never' not a 'sometimes yes sometimes no' kind of control, which in my mind is fundamentally different.)

This is why there is code review.

In a code review it isn't going to be obvious which configurations the author had active while they were developing the PR. So unlike today, a review won't be able to trust that the author would have seen any valid diagnostics in the code (if it's conditionally included). That sounds like a really bad idea.

But I'm glad you brought this up because it reminded me that we also need to consider the behavior in a build environment. I don't like any of the existing options in that environment. I don't think users will be happy to have build systems that don't do any analysis except under one configuration.

There is a precedent in Rust ...

That's good to know, but it doesn't mean that Rust is doing a good job or that we shouldn't try to do better.

[scheglov]

Yes, on CI the package must be tested with all combinations of configurations.
Often this means running in different runtime environment - different CPU, OS, etc.

As I see this, we could allow defining a sets of configurations, again maybe in analysis_options.yaml; and the dart analyze should be able to analyze the package with every configuration, one by one. But in IDE you can have only one set enabled.

You might think that "the users" of this feature is a wide auditory. I suspect that there will be a small number of power users who write multi-platform packages, and they might be capable to keep in mind that if they write such code, they need to validate it on all platforms.

[bwilkerson]

I suspect that there will be a small number of power users ... capable to keep in mind ...

That's likely to be true. And maybe the right answer is option (1) because there are too few users for us to do more.

[lrhn]

"what would be required to completely solve the problem"

It's what I mean. It would be nice, but it's not a requirement.
The Dart language is specified so that it gives a single meaning to a single, stand-alone Dart program and a single provided compilation environment.

Anything else is not the language, it's just SDK tooling.