[analyzer] Analyzer doesn analyze part of the library
#54661
Comments
sgrekhov
added
the
area-analyzer
|
(I haven't performed any experiments with file names, assuming it is all That's surprising! How would // test2.dart
library test_lib;
part 'part.dart';
var foo2; // No name clashes.
main() {
foo2 = 1;
print(bar);
} |
|
Sorry for the typo. I mean |
I'm assuming you mean that you think these diagnostics should be reported when analyzing the library that includes the part ( The documentation isn't clear on this point, but when you use
It doesn't. It gets very confused. That's a big part of why we now allow, and encourage all users to use, URIs in the |
@eernstg do you agree? |
|
Thanks everyone! This looks complicated... Per Sergey's comment (and the PR link), I'm coming at this from the point of view of the language tests, where I am changing the test runner to pick up error expectations from files beyond just the main test file. compilation_t01.dart is a relevant test, it imports a broken part (duplicate definition) and so we can think about whether the error is reported in the part. The command line used by the language tests does report the error: and equivalent output with Then,
Where it gets interesting is
which does find and report the error, despite there being no single owner library. Playing around a bit it looks like it takes the first alphabetical owner library and ignores the rest; so if I add a duplicate definition to Aside: amidst all this complexity there is a trivial bug affecting the output, which is that the JSON gets the file of the context message right but the human-readable version does not: Okay, now some thoughts on what we should actually do here ;) The language tests are not testing Then I think there are three things we need to do:
How does that sound, please? :) |
|
My testing also confirms all of the above. If we preserve the current behaviour of the tools, then, I think, with the updated test runner the // test1.dart
import 'part1.dart`;
main() {...}
// part1.dart
library some_lib;
Line of code containing error;
//^
// [analyzer] unspecified
// [cfe] unspecified
// test2.dart
library test_lib;
part 'part.dart';
main() {
...
}
// part.dart
part of test_lib;
Line of code containing error;
//^
// [cfe] unspecifiedFor |
|
It's true that Maybe this part of the discussion would better belong on the test_runner feature request, but we seem to have mostly the same people anyway :) |
|
@eernstg @bwilkerson @munificent not sure who is the right person / people to decide on what the language tests do here? Input appreciated please :) thanks! |
I'm sure it's "people", not just "person". And I think I'm the right representative from the analyzer team. But I agree we need someone from the language team and maybe someone from the infrastructure team, depending on how big a change we might want there.
The language tests are currently using the It's possible that we need to support reporting diagnostics from part files as part of the same support (that is, we could consider reporting diagnostics from part files only when in "test runner" mode).
I think this needs to take into consideration that the answer might differ depending on whether we're asking about the behavior for users or the behavior for the test runner. The behavior isn't consistent with how the tests are currently written, so we either need to change
I know that using the same part file from multiple libraries is convenient because it means fewer duplicated files, making both the writing and maintaining of tests easier. But there isn't, as far as I'm aware, any valid language semantics for this kind of sharing, which means that the analysis server can't be used when editing these files. If we'd like people to be able to open the And while we're at it, I'd like to add one more factor to the discussion: how are we planning on writing tests for macros and library augmentations (with appropriate caveats: assuming we support macros / library augmentations)? Are we going to want to analyze the augmented library and have diagnostics from the library augmentation(s) be reported, or will we analyzer the library augmentations separately? (My current expectation is that
Yes please. That's a bug no matter what else we decide. I don't know how much impact it's having on users, but we should at least record it so that it isn't forgotten. |
|
Thanks Brian! Some thoughts from my side.
SGTM
That all sounds fine--as far as I can see it does not need to block extending the language tests using the current (deprecated) Stepping back a bit--is there consensus that we want the language tests to cover (and so, be able to assert on) diagnostics in parts?
Having the tests stop using multi-owner parts, except where that is intentional to add test coverage for it, sounds reasonable to me and would certainly simplify the question. @sgrekhov what do you think please, would that be feasible/worthwhile?
I expect we'll want the language tests to have at least the option to cover all the types of diagnostics that might fire, whenever they fire; so I think, yes, a "report all the diagnostics" mode would be good.
Done: #54717 Thanks. |
|
Given: // a.dart
import 'b.dart';
import 'c.dart';
main() {
foo();
}
// b.dart
void foo() {
print('b foo');
}
// c.dart
void foo() {
print('c foo');
}If I run: $ dart analyze a.dartI get: So here I have an example where I'm analyzing a single file that depends on others. The other files have no errors, but in the context of the file I'm analyzing, they lead to an error. The behavior I see here makes sense to me. Given that, I don't see why analyzer isn't reporting an error when you analyze a single file that ends up containing an error because of a part file that it includes. I understand this is "working as intended", but why is this the intended behavior? |
It's not a problem to update the tests to not use multi-owner parts. I don't think that we have more than 1-2 such tests. But, anyway, we should add the special test for this situation. I'm now waiting for this thread to update the tests accordingly |
I don't remember the discussion (assuming that there was one), but I suspect that it's because we were thinking of the command-line tool as operating on files. Given that mindset, the following seems natural:
It's been suggested above that perhaps we should have been thinking about the tool operating on libraries rather than files, and I suppose that's a possibility, but it does raise some questions related to both generated files and uncommon directory structures that the more simplistic approach provides a clear answer for. |
|
I've been OOO a few days, and this issue doesn't seem to have a straightforward solution, so I didn't get around to say anything here for a while. I still think it's difficult. ;-) However, let me outline the main point as I see it: Assume that static analysis is performed on a part file whose "owner" library isn't known (because it uses a In this situation there is no meaningful limit on the discrepancies that may occur relative to a program where that part is actually included: Name resolution may differ (an identifier may refer to an inherited variable declaration according to the analyzer, but it refers to a top-level function declaration in the actual program, or vice versa; or the name may be undefined in one case and defined in the other). Expressions may then have completely different types (and semantics) according to the analyzer and in the actual program. Consequently, compile-time errors may exist in one case, but not in the other, or they may be completely different. In other words, this kind of static analysis may be highly misleading. For this reason, I'd recommend that this kind of static analysis is clearly classified as such. For example, if the heuristic is being used then a diagnostic message is emitted. It would be unfortunate if anyone gets the impression that the static analysis of Dart is unreliable, based on an encounter with this kind of situation. The diagnostic message would inform the developer that a specific library is being considered as the one that has this part file as a part. The question is what to do if the choice differs from the developer's intentions. We already have the following behavior: // --- Library 'test1.dart'
library test_lib;
part 'part.dart';
var foo2;
main() {
foo2 = 1;
print(bar);
}
// --- Library 'test2.dart'.
library test_lib;
part 'part.dart';
var foo;
main() {
foo = 1;
print(bar);
}
// --- Part 'part.dart'.
part of test_lib;
final foo = "foo";
final bar = "bar";That is, we can coerce the analyzer into choosing a specific library/part binding by passing the library as well as the part as command line arguments. However, the fact that there are 'no issues' in all other cases illustrates that it is a somewhat tricky exercise. I believe that a diagnostic message ("this library/part binding was chosen heuristically") could be helpful for developers who are using parts with non-URI |
|
@bwilkerson wrote:
Yes, that's a very interesting idea! I don't know if it would be too disruptive to report all errors on a library and its parts as belonging to the library (that is, they are reported if and only if diagnostics are reported for that library), but I think it would deal with the underspecified library/part binding once and for all. An IDE could then show all messages for the library and all its parts with each of those files (the library and each of the parts), if it is configured to show diagnostic messages for "current file only". But the most important part, I'd say, is still the status of the library/part binding: If it is chosen heuristically then it should be communicated to the developer that it might yield completely misleading results. |
|
Hi everyone, There are some quite tangled issues here :) My own interest is to get unblocked on this PR expanding the assertion power of the static error tests in the language test suite to include files beyond just the test file. To that end I wonder if we can cut through the issues just for the case of these tests by saying that we want the static error tests to have maximal assertion coverage: that we would like the asserts to be about the union of diagnostics the analyzer can produce if used correctly, via any entrypoint, on the test case. This seems to me to be the right thing to do to maximize the value of the tests. The case of multi-owner parts we would handle pragmatically by refactoring those cases so we are back in the territory of defined behaviour. I think that would be enough to unblock me. Even if my PR is not perfect--maybe I missed some case where we could usefully assert on a diagnostic and the tests still don't cover it--it certainly does expand coverage to new useful places; and with the stated intention that we maximize assertion coverage, we can fix any such cases if as we find them. @eernstg @bwilkerson @munificent how does that sound, please? :) |
Sounds good! I'm not 100% sure I understand what it will do, though. ;-) I'd interpret 'entry point' to mean the same thing as 'script' in the language specification, i.e., a library that exports a (If we're supposed to obtain error messages from a part then the part should be specified as a target as well, along with exactly one library that has this part file as a part. But I guess all the part issues have been postponed right now.) |
Thanks for the reminder. I had kind of lost sight of the need for a quicker answer. The test running is currently using the The interesting question is what we need to do in the longer term to |
|
To confirm.
|
|
I think it would be nice to avoid the multi-owner-part issue for now. We could do that by changing every test where the part does not have a unique owner such that it has a unique owner. We should probably still have both About assuming that the analyzer doesn't analyze parts: We do know that |
I would characterize this differently. I think that the upshot is that the test framework and There is an ongoing discussion to figure out what the correct behavior is for the future, but there isn't yet consensus on what that will be. |
|
I'm ready to update co19 tests according to the above. @davidmorgan is it possible rebase https://dart-review.googlesource.com/c/sdk/+/345541? Otherwise it produces an error during the build of the SDK and I can't use the change for the testing |
|
Sounds good @sgrekhov, thanks! Done. I made a few more tweaks to the PR that are not ready for review yet, in particular to show the file name in failure output, but they should work for your testing. |
|
Dart specification (19.5. Parts)
Doen't the specification mean that the contents of the |
It's somewhat vague. It could be interpreted to mean that the tools must report a not-so-informative error message like " So we probably just need to decide on an approach here, and then I wouldn't be surprised if that approach is already one of the possible interpretations of the given section in the language specification. @bwilkerson, what do you think about @davidmorgan, would your changes to the test runner be compatible with this approach? |
Yes, that matches. Thanks. |
Conceptually I have no problem with that. We had convinced ourselves that that wasn't what users want, but if we think that users (other than the test framework) would rather see all the diagnostics then I'm fine with that. (The caveat is there because I'm not willing to compromise the UX to satisfy the test framework when there are other ways for the test framework to be satisfied.) However, doing so might be an interesting implementation challenge. I say that because the way the command-line
If we were talking about a compiler I'd agree. But the analyzer isn't a compiler (as much as it looks like a compiler sometimes). It's the support for our IDE tooling (via the language server). The analyzer does a lot of work to recover from errors in the code. In this particular case, we tolerate a part file being included in multiple libraries by treating each of the enclosing libraries as if it were the sole owner of the part and performing analysis from that perspective [1]. In other words, we support a level of ambiguity (for the sake of the UX) that compilers don't need to tolerate. (Though I think it's fair to say that the compiler effectively tolerates the same ambiguity in the same way as long as only one of the owning libraries is in the program being compiled, as evidenced by the multiple tests that are sharing a single part.) [1] We actually go a little too far in that direction. We don't produce a diagnostic telling the user that the part is being included in multiple libraries, which we ought to do. |
Interesting! But then I'd say that the ambiguity can be eliminated after all: We have one analysis based on ( The missing bit is that every error which is reported for a location in a part file needs to be emitted together with information about which owner library is the chosen one for this analysis result. This would make some error messages more verbose, but if candidate owner libraries are only selected from the same directory, or the same package, or some other "small" set of libraries then it should be possible to give that information in a rather compact form. We still can't know that we've found every library which can be the owner of any given part file (we can't search the entire planet), but this might be handled by adding some constraints ("it is a compile-time error to have a For the test runner it still seems natural to me if errors in a part file are reported together with errors in the owning library. |
The spec has traditionally avoided introducing the notion of a "package", but I wouldn't be surprised if that just failed to work in general.
For the test runner I expect that it would be natural to report every diagnostic from every reachable file (which would be trivial to implement with what we have today. That's part of the reason that I want to separate the question of what the test runner should do from the question of how the command-line tool should work when invoked directly. |
|
@bwilkerson wrote:
This sounds great! Would it be a breaking change? Who would need to agree before we can proceed and just do that? @davidmorgan as well, WDYT? |
Given that we're only talking about the way There's another piece of functionality that we need in My concrete proposal, then, is that we (not necessarily someone on the analyzer team) implement batch mode in |
@bwilkerson if this a trivial change could you please advice how to do it (which files/classes to see)? I want to try to implement this feature |
@sgrekhov my intention is to do that as part of my already-in-progress PR for the test runner, which I believe will make the analyzer asserts match the CFE asserts, for the simplest possible testing. |
Ok, thank you! |
srawlins
added
the
type-bug
We have the following
Analyzer:
dart analyze test.dartreports no issues.dart analyze part.dartreportsThe name 'foo' is already defined.VM:
dart test.dartreportsAnalyzer should report that
part 'part.lib';has some problems preventing executionTested on
Dart SDK version: 3.4.0-edge.ffe2d1cf84bda4f4edcc9550817b7c033c76f8f8 (main) (Fri Jan 12 10:33:53 2024 +0100) on "linux_x64"The text was updated successfully, but these errors were encountered: