Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lexical scope comes before inherited scope #641

Closed
peter-ahe-google opened this issue Nov 30, 2011 · 19 comments
Closed

Lexical scope comes before inherited scope #641

peter-ahe-google opened this issue Nov 30, 2011 · 19 comments
Labels
closed-not-planned Closed as we don't intend to take action on the reported issue web-dart2js

Comments

@peter-ahe-google
Copy link
Contributor

$ cat fisk.dart
int x = 89;

class Foo {
  int x = 42;
}

class Bar extends Foo {
  m() => x;
}

main() {
  print(new Bar().m());
}

$ frogsh fisk.dart
42
$ dartc_test fisk.dart
89
$ cat fisk.dart

class x {
}

class Foo {
  int x = 42;
}

class Bar extends Foo {
  m() => x;
}

main() {
  print(new Bar().m());
}
$ frogsh fisk.dart
42
$ dartc_test fisk.dart
fisk.dart:9: x is a class and cannot be used as an expression
     8: class Bar extends Foo {
     9: m() => x;
Compilation failed with 1 problem.

FWIW, there are a bunch of tests in dart/compiler/javatests/com/google/dart/compiler/{resolver,type}. Karl is already working on porting the tests from the type directory.

@jmesserly
Copy link

Set owner to @jmesserly.
Added Started label.

@jmesserly
Copy link

The VM prints 42 as well.

Gilad, can you give me a sanity check here. Is this is a bug in both implementations? Or misinterpretation of the spec?

Personally--I would really like to keep the current behavior of the VM and Frog. I find it much less surprising to bind to inherited names before top-level names, and I don't like how it makes things from the superclass different from names in my own class. I fear if I make this change to Frog, we'll end up with style rules like "always use 'this.' before members" and/or "never use top level names". That wouldn't be a good result.


Set owner to @gbracha.
Removed this from the FrogEditor milestone.
Removed Area-Frog label.
Added Area-Language, Triaged labels.
Changed the title to: "Lexical scope comes before inherited scope?".

@jmesserly
Copy link

The way I was thinking about this (correctly or not):
  class Bar extends Foo {
    m() => x;
  }

because Bar "extends Foo", Bar gets all the members of Foo, therefore "x" is in its lexical scope. By contrast, members of subtypes of Bar (if any) don't resolve in Bar. That's how I recall the discussion going last time this came up.

@peter-ahe-google
Copy link
Contributor Author

In the future, please follow this process for requesting specification clarification:

  1. File a new issue.
  2. Mark the current bug as blocked on this issue.

Otherwise, the new owner of the bug is likely to forget that he should not close the bug but reassign back. If there are further follow-up questions a bug can fluctuate between various areas and tend to be forgotten when you perform bug scrubs. You may also loose labels in the process.


Removed the owner.
Added this to the FrogEditor milestone.
Removed Area-Language label.
Added Area-Frog label.

@gbracha
Copy link
Contributor

gbracha commented Dec 6, 2011

The answer is 89. Lexical scope has priority over inherited scoped. The reason for this is mainly to bullet proof us against future features that might come up, like class nesting, first class libraries and how they interact.

@jmesserly
Copy link

Bullet proofing future features is definitely good :)

Just to double check my understanding--does "lexical scope" include top level names from other source files and imported libraries too, or just the current file?

I'm mainly worried from two angles: how do I bullet proof code within my own Dart classes from accidentally referring to something it didn't intend? And as a library designer, how do I add a new top level name without breaking someone else's class members? It seems like every top level name has the potential to conflict with any member variable/function. We're okay on type names because our UpperCamelCase names are unlike to conflict with lowerCamelCase member names, but for things like "document" "window" etc, what should we do?

@peter-ahe-google
Copy link
Contributor Author

"Lexical" scope does include top-level names from everything #source'd and #import'ed into the current library.

We recommend importing stuff you're not in control of with prefixes. This way you reduce the "lexical" scope.

We should warn when there are conflicts (Gilad is looking at a minor tweak to the specification of this). The issue is what happens when you ignore the warnings. We need to have a tie-breaker rule and our experience is that the letting the superclass take precedence or lexical scope is not helpful.

@jmesserly
Copy link

Importing everything by prefix is not a solution. Sure you can work around the surprising conflict resolution rules like that--but I'd rather get the conflict resolution rules right, and not have to work around it. I can't imagine trying to sell Java developers on qualifying type names, much less JS devs or anyone from a more dynamic language background.

RE scoping: isn't the whole point of declaring a superclass of a class "bring these names and functions the scope of my object"?

@munificent
Copy link
Member

We recommend importing stuff you're not in control of with prefixes. This way you reduce the "lexical" scope.

I've heard you say that before, but it's in contradiction with our practices. If you look through all of our existing code it almost never uses prefixes.

We should warn when there are conflicts (Gilad is looking at a minor tweak to the specification of this).

I like this. I don't see much reason to use prefixes when there aren't name collisions anyway.

@jmesserly
Copy link

oops--I forgot the word "into" above, that should've read:
"bring these names and functions into the scope of my object"

@jmesserly
Copy link

Here's another thought: if we want to preserve maximum flexibility for future features, why not make conflicts between inherited and lexical scope be an error? Means we'd never have to guess which name the user wanted and surprising them if they are used to this working in other languages.

Maybe we should also change the coding style guide to either say "avoid top-level functions in libraries", and/or "derived classes should always use 'super.' to access anything from the parent class".

BTW, I tried to construct a similar example in C#, but the rules of the language make it pretty hard--there's no top level functions and I couldn't figure out how to make it ambiguous about whether you wanted a class name or a member name. But if I create "class Foo {}" it doesn't conflict with methods called "int Foo()" in other classes (upper case member names are the normal C# convention).

The example translates quite easily to Ruby, where inherited scope wins:

def x(); 89; end
class Foo
  def x(); 42; end
end
class Bar < Foo
  def m(); x; end
end
class Baz
  def m(); x; end
end
puts(Bar.new.m) # prints 42
puts(Baz.new.m) # prints 89

(Of course, Python/JavaScript avoid this problem by requiring explicit "self."/"this.")


Changed the title to: "Lexical scope comes before inherited scope".

@dgrove
Copy link
Contributor

dgrove commented Jan 10, 2012

Can you open a language bug for the feature discussion?

@kasperl
Copy link

kasperl commented Jan 13, 2012

Removed this from the FrogEditor milestone.

@DartBot
Copy link

DartBot commented Jan 13, 2012

This comment was originally written by jimhug@google.com


I'm still very interested in the language discussion continuing on this one. However, I'm going to try to fix this as part of a related cleanup to Value today - mainly to see how I feel about the semantics after I've implemented them <smile>. Any pointers to automated versions of these tests that are part of one of the standard test suites would be very much appreciated if available.


Set owner to jimhug@google.com.
Added Started label.

@rakudrama
Copy link
Member

I don't understand the motivation for the lookup rule.

As per Peter's request I have opened Issue #1180 as a language issue, so this issue can stay focused on Frog's conformance.


Marked this as being blocked by #1180.

@sethladd
Copy link
Contributor

sethladd commented Feb 9, 2012

I started writing an article on lexical scopes, but found that the VM still prints 42. Is there an issues specific to the VM for this? This issue is only for Frog.

@peter-ahe-google
Copy link
Contributor Author

I've created issue #1598 to track the VM bug.


Removed the owner.
Added Triaged label.

@anders-sandholm
Copy link
Contributor

Removed Area-Frog label.
Added Area-Dart2JS, FromAreaFrog labels.
Marked this as being blocked by #1180.
Unmarked this as being blocked by #-1180.

@kasperl
Copy link

kasperl commented Jun 12, 2012

Added WontFix label.

@kevmoo kevmoo added closed-not-planned Closed as we don't intend to take action on the reported issue and removed resolution-wont_fix labels Mar 1, 2016
copybara-service bot pushed a commit that referenced this issue May 20, 2022
Changes:
```
> git log --format="%C(auto) %h %s" c1eb6cb..b149f80
 https://dart.googlesource.com/protobuf.git/+/b149f80 Remove the top level analysis_options.yaml file (#656)
 https://dart.googlesource.com/protobuf.git/+/8f6f307 Fix comment syntax
 https://dart.googlesource.com/protobuf.git/+/665f7b0 Remove trailing whitespace in protobuf/LICENSE
 https://dart.googlesource.com/protobuf.git/+/9ffbaf2 Fix default list type for frozen messages (#654)
 https://dart.googlesource.com/protobuf.git/+/a68bb5a Fix Fixed32 to be parsed as unsigned when parsing proto3 protos (#655)
 https://dart.googlesource.com/protobuf.git/+/6957c98 Convert the field started with underscore and digit to a legal name (#651)
 https://dart.googlesource.com/protobuf.git/+/71defca Account for double.infinity and double.nan in json serializers (#652)
 https://dart.googlesource.com/protobuf.git/+/5a48349 Allow `Timestamp.toDateTime()` to return a `DateTime` in local timezone (#653)
 https://dart.googlesource.com/protobuf.git/+/117e869 Make the toString of enums be the value if names are omitted (#649)
 https://dart.googlesource.com/protobuf.git/+/88c4016 Align the hashCode of a message with an empty unknown field-set with that of no unknown field set (#648)
 https://dart.googlesource.com/protobuf.git/+/eed09c4 Fix proto3 repeated field encoding without packed option (#635)
 https://dart.googlesource.com/protobuf.git/+/8f587b1 Simplify `_FieldSet` getters (#646)
 https://dart.googlesource.com/protobuf.git/+/494f189 Fix compile-protos.sh interpreter (#645)
 https://dart.googlesource.com/protobuf.git/+/5e1a422 Fix typo in protobuf-0.9.0+1 changelog
 https://dart.googlesource.com/protobuf.git/+/46df68a Add `UseResult` annotation to `rebuild` (#631)
 https://dart.googlesource.com/protobuf.git/+/ff5304f Migrate protoc_plugin to null safety (#642)
 https://dart.googlesource.com/protobuf.git/+/657197d Fix typo in GeneratedMessage.copyWith documentation (#641)
 https://dart.googlesource.com/protobuf.git/+/e30a522 Fix sharing coded buffer bytes when parsing `bytes` fields (#640)
 https://dart.googlesource.com/protobuf.git/+/810b166 Clear unknown field when setting an extension field with the same tag (#639)
 https://dart.googlesource.com/protobuf.git/+/d30623b Treat empty and uninitialized Maps the same in equality checks (#638)
 https://dart.googlesource.com/protobuf.git/+/4fe3ee4 Make `MapFieldInfo` key and value types non-nullable (#600)
 https://dart.googlesource.com/protobuf.git/+/c26ac34 Add grpc example to protoc_plugin README (#514)
 https://dart.googlesource.com/protobuf.git/+/c35d787 Revert changes to reserved names to maintain backwards compat (#636)
 https://dart.googlesource.com/protobuf.git/+/146b186 Remove unused `GeneratedMessage` constructors (#634)
 https://dart.googlesource.com/protobuf.git/+/1b12ac9 Remove a closure in `_FieldSet.hashCode` (#633)
 https://dart.googlesource.com/protobuf.git/+/5731242 Minor fix in query_bench set_nested_value benchmark: (#630)
 https://dart.googlesource.com/protobuf.git/+/767ce81 Remove fold() closures from `FieldSet._hashCode`. (#554)
 https://dart.googlesource.com/protobuf.git/+/99bc541 protoc_plugin README fixes and tweaks: (#617)
 https://dart.googlesource.com/protobuf.git/+/e282e17 protobuf benchs: invoke protoc once with all protos (#623)
 https://dart.googlesource.com/protobuf.git/+/bef672b protobuf benchs: fix old --trust-type-annotations flag (#622)
 https://dart.googlesource.com/protobuf.git/+/d072e5f dependabot: check for updates monthly (#620)
 https://dart.googlesource.com/protobuf.git/+/96bdf38 Fix TypeRegistry passing when unpacking nested Any messages from JSON (#568)
 https://dart.googlesource.com/protobuf.git/+/4ec722a Correctly combine hash codes for repeated enums. (#556)
 https://dart.googlesource.com/protobuf.git/+/b96dc21 Testing: don't return static methods in findMemberNames (#618)
 https://dart.googlesource.com/protobuf.git/+/09e8a8d Update protobuf/benchmarks README for #553
 https://dart.googlesource.com/protobuf.git/+/3ef4539 Add a benchmark for computing hashCodes. (#553)
 https://dart.googlesource.com/protobuf.git/+/d232e6e Tweak READMEs: (#610)
 https://dart.googlesource.com/protobuf.git/+/0f01fa9 Update protobuf pubspec.yaml (#616)
 https://dart.googlesource.com/protobuf.git/+/a10426b Update protoc_plugin pubspec.yaml (#615)
 https://dart.googlesource.com/protobuf.git/+/a0021c7 Make `protoName` unCamelCase lazy (#606)
 https://dart.googlesource.com/protobuf.git/+/ded1ac7 Document map key and value fields (#603)
 https://dart.googlesource.com/protobuf.git/+/8792f2a Remove redundant check in PbMap equality check (#604)
 https://dart.googlesource.com/protobuf.git/+/b7f9569 Tweak BuilderInfo and FieldInfo docs: (#597)
 https://dart.googlesource.com/protobuf.git/+/6f85c32 Remove a redundant cast (#598)
 https://dart.googlesource.com/protobuf.git/+/3df8669 Latest mono_repo (#601)
 https://dart.googlesource.com/protobuf.git/+/9da84ae Fix a potential issue in CodedBufferWriter (#594)
 https://dart.googlesource.com/protobuf.git/+/6be405f Remove old and unused test file (#589)
 https://dart.googlesource.com/protobuf.git/+/900cef5 Fix protoc_plugin run-tests make rule (#586)
 https://dart.googlesource.com/protobuf.git/+/2546269 Fix rounding when handling negative timestamps (#580)
 https://dart.googlesource.com/protobuf.git/+/8afce8d Fix Readme `pub get` instead of `pub install`. (#486)
 https://dart.googlesource.com/protobuf.git/+/782fd24 Avoid runtime function type check in lazily created singleton creator functions (#574)
 https://dart.googlesource.com/protobuf.git/+/a7e75cb Update README.md
 https://dart.googlesource.com/protobuf.git/+/23136dc Version bump (#563)
 https://dart.googlesource.com/protobuf.git/+/18346f5 Convert null keys and values to default when parsing map entries (#536)
 https://dart.googlesource.com/protobuf.git/+/bb4cf0b Bump to latest mono_repo - use latest actions (#561)
 https://dart.googlesource.com/protobuf.git/+/835ab75 Remove unneeded imports
 https://dart.googlesource.com/protobuf.git/+/ef733ac latest mono_repo
 https://dart.googlesource.com/protobuf.git/+/ecfb862 Fix test for latest analysis (#543)
 https://dart.googlesource.com/protobuf.git/+/26a0a26 fix insecure link in protoc_plugin readme
 https://dart.googlesource.com/protobuf.git/+/444e855 Bump dart-lang/setup-dart from 1.1 to 1.2 (#535)
 https://dart.googlesource.com/protobuf.git/+/d3e0e4a Fix analysis options
 https://dart.googlesource.com/protobuf.git/+/cf29280 Fix comment references

```

Diff: https://dart.googlesource.com/protobuf.git/+/c1eb6cb51af39ccbaa1a8e19349546586a5c8e31~..b149f801cf7a5e959cf1dbf72d61068ac275f24b/

Tested: relying on existing SDK and protobuf tests
Change-Id: I7f89b998a0aba13999d180ee9814a26a5f1d054d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/245228
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Ömer Ağacan <omersa@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-not-planned Closed as we don't intend to take action on the reported issue web-dart2js
Projects
None yet
Development

No branches or pull requests