Support for show/hide combinators

Support for show/hide combinators

- Add a new VM object called a Namespace.
- A Namespace is a library, and two list of names to hide/show.
- Convert Library and LibraryPrefix to contain Namespace
  objects instead of Libraries in their import list.
Committed: https://code.google.com/p/dart/source/detail?r=12804

[hausner, 2012-09-21 21:59:27]

The review is not as big as it looks. Lots of boilerplate due to the new VM
class Namespace.

[siva, 2012-09-24 18:28:51]

Looks good overall but I have some spec questions.

1. Is this legal?
import "xyz.dart" as xyz hide A,B;
import "xyz.dart" as xyz show A hide B;
If this is legal I presume the last one takes precedence
but in the current implementation the first one is applied.
I think this scenario can also occur with normal imports with different
combinators.

2. Is this legal?
import "xyz.dart" show _file;
where _file is a private variable.
Does this mean the private variable now becomes visible?
If not do we need to flag an error?

3. Is this legal?
import "xyz.dart" hide A show A;
If so does the show take precedence over the hide?
In the current implementation whatever comes first seems to
take precedence and that is not very intuitive.

4.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/code_generator_t...
File runtime/vm/code_generator_test.cc (right):

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/code_generator_t...
runtime/vm/code_generator_test.cc:273: lib.AddImportAll(core_lib);
AddImportAll seems like a bad name why not
AddImportWithNoCombinators or create a name space here
with empty combinators and just call
AddImport

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6328: // libs.
The comment in LookupFieldAllowPrivate has been modified but not this one?

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6367: // Now check if name is found in the top level scope
of any imported libs.
ditto comment regarding comment.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6527: AddImport(ns);
I would prefer if we did not have his convenience function and instead made the
callers create a NameSpace with null combinators and just call AddImport.

Would avoid duplicate handle creation (see code in Library::InitCoreLibrary)

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6839: intptr_t num_current_imports = num_imports();
This seems to imply that a library can be imported with the same prefix but
different show/hide combinators. I presume
that means the last import statement is the one that takes precedence.

The implementation though is not quite like that as we go
over the imports from 0 .... length in lookups.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4182: ParseIdentList(&show_list);
From this code it appears that one can add private names
to the show list, not sure what the spec states in terms of
whether that should be flagged as an error or not.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4189: }
Another question there is no check to ensure that the names being listed in the
show/hide list exist in the library.

Can a name appear in both the hide list and show list? if the show list appears
after the hide list and a name appears in both does it mean the show takes
precedence.

I am sure we will have builders complaining that they included a name in the
show list yet get errors (name misspelt in the list).

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:8032: if (ns.HidesName(name)) {
The name HidesName is not very readable here as it seems to imply we are looking
in the hides list and not considering the show list.

maybe FiltersName would be better.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:749: RawArray* imports_;             // libraries
imported with this prefix.
Comment should be namespaces imported with this prefix?

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:753: intptr_t num_imports_;          // Number of
library entries in libraries_.
Comment should be number of namespace entries in imports_?

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:765: RawArray* hide_names_;         // blacklist of
names that are not exported.
why blacklist?

list of names that are not exported.

http://codereview.chromium.org/10967052/diff/8001/tests/language/import_combi...
File tests/language/import_combinators_negative_test.dart (right):

http://codereview.chromium.org/10967052/diff/8001/tests/language/import_combi...
tests/language/import_combinators_negative_test.dart:7: import
"import1_lib.dart" show hide, show hide ugly;
STV

http://codereview.chromium.org/10967052/diff/8001/tests/language/import_combi...
File tests/language/import_combinators_test.dart (right):

http://codereview.chromium.org/10967052/diff/8001/tests/language/import_combi...
tests/language/import_combinators_test.dart:7: import "import1_lib.dart" show
hide, show hide ugly;
Would have been better if you had:

import "import1_lib.dart" as show show hide, show hide ugly;

main() {
  print(show.hide);
  print(show.show);
}

[hausner, 2012-09-24 20:25:43]

Thanks for the careful review. I'll press for a more precise spec.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/code_generator_t...
File runtime/vm/code_generator_test.cc (right):

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/code_generator_t...
runtime/vm/code_generator_test.cc:273: lib.AddImportAll(core_lib);
On 2012/09/24 18:28:51, siva wrote:
> AddImportAll seems like a bad name why not
> AddImportWithNoCombinators or create a name space here
> with empty combinators and just call
> AddImport

Done.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6328: // libs.
On 2012/09/24 18:28:51, siva wrote:
> The comment in LookupFieldAllowPrivate has been modified but not this one?

Done.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6367: // Now check if name is found in the top level scope
of any imported libs.
On 2012/09/24 18:28:51, siva wrote:
> ditto comment regarding comment.

Done.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6527: AddImport(ns);
On 2012/09/24 18:28:51, siva wrote:
> I would prefer if we did not have his convenience function and instead made
the
> callers create a NameSpace with null combinators and just call AddImport.
> 
> Would avoid duplicate handle creation (see code in Library::InitCoreLibrary)

Done. (Although I think the convenience function was nicer.)

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6839: intptr_t num_current_imports = num_imports();
The spec does not say what happens if the combinators contradict each other. We
always have to go through all imports so that we detect ambiguous names. Gilad
said if you import the same name twice (explicitly) then you should get an
error. But these things change all the time, so I think we are fine for now as
the code is.

On 2012/09/24 18:28:51, siva wrote:
> This seems to imply that a library can be imported with the same prefix but
> different show/hide combinators. I presume
> that means the last import statement is the one that takes precedence.
> 
> The implementation though is not quite like that as we go
> over the imports from 0 .... length in lookups.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4182: ParseIdentList(&show_list);
Private names are not contained in the export namespace of the exporting
library, so I expect "show _A" to be a no-op, just like "show NonExistingName".

On 2012/09/24 18:28:51, siva wrote:
> From this code it appears that one can add private names
> to the show list, not sure what the spec states in terms of
> whether that should be flagged as an error or not.

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4189: }
I added a note to the spec asking for clarification about contradicting
combinators. Regarding non-existing names, I interpret the spec as it being a
no-op if you show/hide a non-existing name.

On 2012/09/24 18:28:51, siva wrote:
> Another question there is no check to ensure that the names being listed in
the
> show/hide list exist in the library.
> 
> Can a name appear in both the hide list and show list? if the show list
appears
> after the hide list and a name appears in both does it mean the show takes
> precedence.
> 
> I am sure we will have builders complaining that they included a name in the
> show list yet get errors (name misspelt in the list).

http://codereview.chromium.org/10967052/diff/8001/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:8032: if (ns.HidesName(name)) {
FiltersName does not say what the filter is doing. A name is hidden if it either
appears in the 'hide' list, or if it is not explicitly listed in the 'show'.

Another options is "IsHidden()" but it seems that we're splitting hairs...

On 2012/09/24 18:28:51, siva wrote:
> The name HidesName is not very readable here as it seems to imply we are
looking
> in the hides list and not considering the show list.
> 
> maybe FiltersName would be better.

http://codereview.chromium.org/10967052/diff/8001/tests/language/import_combi...
File tests/language/import_combinators_test.dart (right):

http://codereview.chromium.org/10967052/diff/8001/tests/language/import_combi...
tests/language/import_combinators_test.dart:7: import "import1_lib.dart" show
hide, show hide ugly;
On 2012/09/24 18:28:51, siva wrote:
> Would have been better if you had:
> 
> import "import1_lib.dart" as show show hide, show hide ugly;
> 
> main() {
>   print(show.hide);
>   print(show.show);
> }

:-)

[siva, 2012-09-24 21:12:37]

lgtm

http://codereview.chromium.org/10967052/diff/16001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/10967052/diff/16001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:8172: String::Handle(lib.url()).ToCString());
We need a "if (first_lib_url.raw() == lib.url())" check
here to avoid weird error messages.

[siva, 2012-09-24 21:20:40]

http://codereview.chromium.org/10967052/diff/16001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/10967052/diff/16001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:453: SET_CLASS_NAME(library_prefix, LibraryPrefix);
oops forgot this:
We are missing
SET_CLASS_NAME(namespace, NameSpace);

and NameSpace added to symbols.h so that people referring
to this class name will get something.

[hausner, 2012-09-24 21:41:06]

See separate chicken^H^H^H^H^Heckin.

http://codereview.chromium.org/10967052/diff/16001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/10967052/diff/16001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:453: SET_CLASS_NAME(library_prefix, LibraryPrefix);
On 2012/09/24 21:20:40, siva wrote:
> oops forgot this:
> We are missing
> SET_CLASS_NAME(namespace, NameSpace);
> 
> and NameSpace added to symbols.h so that people referring
> to this class name will get something.

Done.

http://codereview.chromium.org/10967052/diff/16001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/10967052/diff/16001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:8172: String::Handle(lib.url()).ToCString());
On 2012/09/24 21:12:37, siva wrote:
> We need a "if (first_lib_url.raw() == lib.url())" check
> here to avoid weird error messages.

Done.