Remove package imports from itself.

Remove package imports from itself.

Without this, we risk creating two different libraries at runtime. One
named file:///something/ and one named package:front_end/something.
These libraries are different and if enums from one gets mixed with
the other, you get very confusing errors (to apparently identical enums
aren't equal).

R=johnniwinther@google.com

Committed: https://github.com/dart-lang/sdk/commit/c59ecd801c69b9911c73790d1e8782c15282c385

[ahe, 2017-03-01 15:42:42]

ahe@google.com changed reviewers:
+ karlklose@google.com, paulberry@google.com

[ahe, 2017-03-01 15:42:43]

I only need one of you to review this.

[Paul Berry, 2017-03-01 20:25:54]

We've actually put a lot of thought into this on the analyzer side, because not
only do we need to avoid having a single file reachable by two different URIs
(or, as you say, creating two different libraries pointing to the same file),
but we also need to implement logic in the analysis server so that when the user
activates a quick fix that causes the analysis server to automatically add an
import, it has the least chance of causing this problem for the user.

Here's what we came up with (pardon the length; I copied and pasted much of this
explanation from the last time we discussed the subject):

Strictly speaking it doesn't matter how files within "lib/" refer to each other,
provided that the developer is careful to follow two other best practices:
always use "package:" when a file outside of "lib/" refers to a file inside of
"lib/", and never put a "main" function inside of "lib/".  This works because it
means that all files inside "lib/" are always reached through a "package:" URI. 
(Note that the practice of never putting a "main" function inside of "lib/" is
part of the official pub package layout conventions, and current front_end Fasta
code doesn't follow it, probably because the Fasta subdirectory was imported
from a project that used to be a package all its own). 

However, we found a problem with the rules stated above: they were complicated,
and users had a lot of difficulty remembering what was safe to do and what
wasn't.  So we tried to find a stricter set of rules which were (a) are as
simple as possible, and (b) contained the damage as much as possible if they
were not always followed.

So what we decided to do instead was to tell people to always use a "package:"
URI when referring to a file in "lib/", regardless of where they are referring
to it from.  Not only is that a simpler rule, but if they ever do something they
shouldn't (like put a "main" function inside of "lib/"), then the damage will be
much more contained, because after one hop through the import graph, all files
are reached through "package:" URIs.

At the time we were talking about this, analysis server's algorithm for
auto-adding imports already had the policy of using "package:" imports to refer
to "lib/" regardless of where the importing file was located.  So it seemed
prudent to advise people to use the rule that was (a) simpler, (b) more robust,
and (c) consistent with analysis server's behavior.

The pub package layout conventions I referred to earlier are consistent with
this: they say "When you use libraries from within your own package, even code
in src, you can (and should) still use package: to import them."

What you're doing in this CL interacts poorly with the recommended best
practices, because it means that if someone ever introduces a self-package
import into front end, even just a single one, there is a risk of breaking
things again.  And that's likely to happen because analysis server sometimes
will do this as a quick fix.  Even if we changed analysis server so that it did
not do so, and trained developers to always use relative imports for self
imports, there would still be a problem any time there is a circular reference
between packages (as there currently is between front_end and analyzer).  Those
circular references will always go through "package:" URIs, so if we try to keep
other things using "file:" URIs, there will always be a mismatch.

So I would advise that we drop this CL, and instead move the main() functions
outside of "lib/".  That should be pretty low impact, since all we have to do is
introduce a few files in "bin" which just call the appropriate functions inside
"lib/".

[ahe, 2017-03-02 05:55:39]

On 2017/03/01 20:25:54, Paul Berry wrote:
> Strictly speaking it doesn't matter how files within "lib/" refer to each
other,
> provided that the developer is careful to follow two other best practices:
> always use "package:" when a file outside of "lib/" refers to a file inside of
> "lib/", and never put a "main" function inside of "lib/".

Not having "main" function inside of "lib/" makes good sense to me.
 
> So I would advise that we drop this CL, and instead move the main() functions
> outside of "lib/".  That should be pretty low impact, since all we have to do
is
> introduce a few files in "bin" which just call the appropriate functions
inside
> "lib/".

I see the problem, and I agree that the main functions should be moved out of
"lib/". And I will do that. As far as I can tell, it would be best to start by
moving these to the "tool/" folder.

However, I have a number of CLs on top of this, and I would prefer to deal with
the main functions in a follow-up CL.

[ahe, 2017-03-02 09:30:19]

ahe@google.com changed reviewers:
+ johnniwinther@google.com

[ahe, 2017-03-02 09:30:19]

Following up in CL 2722223006.

@johnniwinther: could you take a look? Karl is out today.

[Johnni Winther, 2017-03-02 09:54:23]

LGTM (together with https://codereview.chromium.org/2722223006/)

[ahe, 2017-03-02 10:16:09]

Thank you, Paul and Johnni.

Paul: let me know if I haven't addressed all your concerns, and I'll follow up
with more CLs.

[ahe, 2017-03-02 11:34:53]

Description was changed from

==========
Remove package imports from itself.

Without this, we risk creating two different libraries at runtime. One
named file:///something/ and one named package:front_end/something.
These libraries are different and if enums from one gets mixed with
the other, you get very confusing errors (to apparently identical enums
aren't equal).
==========

to

==========
Remove package imports from itself.

Without this, we risk creating two different libraries at runtime. One
named file:///something/ and one named package:front_end/something.
These libraries are different and if enums from one gets mixed with
the other, you get very confusing errors (to apparently identical enums
aren't equal).

R=johnniwinther@google.com

Committed:
https://github.com/dart-lang/sdk/commit/c59ecd801c69b9911c73790d1e8782c15282c385
==========

[ahe, 2017-03-02 11:34:54]

Committed patchset #1 (id:1) manually as
c59ecd801c69b9911c73790d1e8782c15282c385 (presubmit successful).

[Paul Berry, 2017-03-02 13:52:38]

On 2017/03/02 10:16:09, ahe wrote:
> Thank you, Paul and Johnni.
> 
> Paul: let me know if I haven't addressed all your concerns, and I'll follow up
> with more CLs.

Thanks, Peter.  Thanks to https://codereview.chromium.org/2722223006/, all my
concerns are addressed.

[ahe, 2017-03-02 13:54:05]

On 2017/03/02 13:52:38, Paul Berry wrote:
> On 2017/03/02 10:16:09, ahe wrote:
> > Thank you, Paul and Johnni.
> > 
> > Paul: let me know if I haven't addressed all your concerns, and I'll follow
up
> > with more CLs.
> 
> Thanks, Peter.  Thanks to https://codereview.chromium.org/2722223006/, all my
> concerns are addressed.

Great. Thanks for the detailed explanation, BTW.