Don't allow a #source directive in a library unit include itself

Don't allow a #source directive in a library unit include itself

Committed: https://code.google.com/p/dart/source/detail?r=383

[jbrosenberg, 2011-10-11 21:32:31]

This is a simple change, to handle the special case where a source that is a
library, has a #source directive which refers to itself.  This issue was
reported by Dan Rubel

http://codereview.chromium.org/8229028/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8229028/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:503: if
(modifiers.isGetter()) {
The changes in this method are some refactoring suggested awhile ago by Miguel
in an older review....

http://codereview.chromium.org/8229028/diff/1/tests/language/src/SourceSelfNe...
File tests/language/src/SourceSelfNegativeTest.dart (right):

http://codereview.chromium.org/8229028/diff/1/tests/language/src/SourceSelfNe...
tests/language/src/SourceSelfNegativeTest.dart:5: // Dart test program importing
the core library explicitly.
This test was suggested by Steve Messick

[jbrosenberg, 2011-10-11 22:05:12]

http://codereview.chromium.org/8229028/diff/1/tests/language/src/SourceSelfNe...
File tests/language/src/SourceSelfNegativeTest.dart (right):

http://codereview.chromium.org/8229028/diff/1/tests/language/src/SourceSelfNe...
tests/language/src/SourceSelfNegativeTest.dart:5: // Dart test program importing
the core library explicitly.
Sorry, it was Dan Rubel who suggested this.

[danrubel, 2011-10-11 22:13:21]

LGTM with 1 suggestion.

http://codereview.chromium.org/8229028/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8229028/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:545:
DartCompilerErrorCode.MISSING_LIBRARY_DIRECTIVE, unit.getSourceName()));
From the user's point of view, should the error be reported on the *.dart file
being imported indicating that it is missing a #library directive? Or is it
better to report the error on the *.dart file with the #import statement
indicating that it is trying to import a file without a #library directive? I'm
guessing that the latter approach is better because (1) the user may have
imported the wrong file rather than the file needing a #library directive, and
(2) the user can hyperlink/jump from the #import to the file being imported, but
the reverse is much more difficult.

[codefu, 2011-10-11 22:14:00]

On 2011/10/11 22:13:53, codefu wrote:

LGTM

[Brian Wilkerson, 2011-10-11 22:29:15]

http://codereview.chromium.org/8229028/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8229028/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:544:
context.compilationError(new DartCompilationError(Location.NONE,
Please, please, please! Always pass in a non-null source when creating a
compilation error (and consider removing the one constructor that doesn't take a
source). The editor uses the source to associate the error with the right file.
Without a source we cannot open the right file when the user double-clicks on an
error in the Problems view.

http://codereview.chromium.org/8229028/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:545:
DartCompilerErrorCode.MISSING_LIBRARY_DIRECTIVE, unit.getSourceName()));
> ... report the error on the *.dart file with the #import statement
> indicating that it is trying to import a file without a #library directive?

As I have argued several times before, the second is *much* better from a user's
perspective. The error is not with the way the file is being declared, it is
with the way the file is being used. We would use the same criteria for
assignments to final fields. It would be hard to argue that the right error
message is "you should not declare this as final because someone wants to assign
to it".

[jbrosenberg, 2011-10-12 14:52:51]

http://codereview.chromium.org/8229028/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8229028/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:545:
DartCompilerErrorCode.MISSING_LIBRARY_DIRECTIVE, unit.getSourceName()));
No disagreement from me. Perhaps we have a bit more time now to update this
error reporting, I'll take a look.  Since I expect there are other places where
we don't report errors clearly, there should probably be a separate followon
effort to deprecate the one method that doesn't take a Source arg, etc.

[Brian Wilkerson, 2011-10-12 15:03:01]

Thanks. I created an issue so that it won't be forgotten
(http://code.google.com/p/dart/issues/detail?id=82).

[mmendez, 2011-10-12 18:29:17]

On 2011/10/12 15:03:01, Brian Wilkerson wrote:
> Thanks. I created an issue so that it won't be forgotten
> (http://code.google.com/p/dart/issues/detail?id=82).

LGTM

[jbrosenberg, 2011-10-12 20:50:18]

PTAL, I think I've made the error reporting, for the directive validation case,
cleaner and it reports the error at the point the directive is declared, and not
in reference to the included file.

[codefu, 2011-10-12 22:21:33]

On 2011/10/12 20:50:18, jbrosenberg wrote:
> PTAL, I think I've made the error reporting, for the directive validation
case,
> cleaner and it reports the error at the point the directive is declared, and
not
> in reference to the included file.

Still LGTM.  
The verification logic could be refactored later to the unit level, in case our
directives change.