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

incompatible_static_name_resolution: Implement Name resolution proposal #5637

Closed
laurentlb opened this Issue Jul 19, 2018 · 8 comments

Comments

Projects
None yet
4 participants

bazel-io pushed a commit that referenced this issue Aug 22, 2018

Distinguish between Module scope and Universe scope in ValidationEnvi…
…ronment

This is just a refactoring, no behavior change intended. This change makes
obvious a previous bug we want to fix. It is also a prerequisite for other
changes related to name resolution.

#5637

RELNOTES: None.
PiperOrigin-RevId: 209761775

bazel-io pushed a commit that referenced this issue Aug 27, 2018

Delete Environment.hasVariable
It's virtually unused, as callers generally want to know what is the value.

#5637

RELNOTES: None.
PiperOrigin-RevId: 210401091

bazel-io pushed a commit that referenced this issue Aug 27, 2018

Make sure lexicalFrame is never null
This simplifies the code a bit.

#5637

RELNOTES: None.
PiperOrigin-RevId: 210410389

bazel-io pushed a commit that referenced this issue Aug 27, 2018

Introduce --incompatible_static_name_resolution
This implements a part of the name resolution document: https://github.com/bazelbuild/proposals/blob/master/docs/2018-06-18-name-resolution.md

When the flag is set:
 - Builtins may be shadowed (e.g. `len = 2`) on top-level
 - Symbols may be used before their definition point, e.g.
      def fct(): return x
      x = 2

In a followup change, we'll need to update the dynamic Environment too (to have truly lexical binding).

#5637

RELNOTES: None.
PiperOrigin-RevId: 210412609

bazel-io pushed a commit that referenced this issue Aug 28, 2018

Add static name resolution
This implements a part of the name resolution document: https://github.com/bazelbuild/proposals/blob/master/docs/2018-06-18-name-resolution.md
and it is enabled with the flag --incompatible_static_name_resolution

There are two visible changes:

1. Local variables can be used before their definition point.
2. Local variables will shadow global variables, even if they are not initialiazed yet.

#5637

RELNOTES: None.
PiperOrigin-RevId: 210562752

bazel-io pushed a commit that referenced this issue Aug 29, 2018

Environment: introduce method for more specific lookups
Callers should know in which frame a value is defined. This is needed for
static name resolution. It avoids extra lookups and will allow other
performance improvements.

#5637

RELNOTES: None.
PiperOrigin-RevId: 210762449

bazel-io pushed a commit that referenced this issue Aug 31, 2018

Environment: ensure that the global frame does at most two lookups
When we try to access a variable, we do a lookup in the environment. The globalframe has a parent field, so we have no limit on the number of lookups. It may do multiple lookups until it finds a value or the parent is null.

With this CL:
- globalframe represents the symbols from the file
- globalframe.parent represents the universe (builtins)
- globalframe.parent.parent won't exist

So it's at most two lookups. Later, we should reduce to just one lookup. After that, we can have further optimizations (e.g. array lookup instead of hashmap lookup).

#5637

RELNOTES: None.
PiperOrigin-RevId: 211143657

bazel-io pushed a commit that referenced this issue Sep 4, 2018

Use the more specialized lookup functions.
#5637

RELNOTES: None.
PiperOrigin-RevId: 211476817
@laurentlb

This comment has been minimized.

Contributor

laurentlb commented Sep 7, 2018

Allow shadowing of builtins in bzl files
395fbbd

@dslomov dslomov changed the title Implement Name resolution proposal incompatible_static_name_resolution: Implement Name resolution proposal Oct 31, 2018

@dslomov

This comment has been minimized.

Contributor

dslomov commented Oct 31, 2018

What is missing for migration: migration docs, length of migration window. After these are done, please add "migration-ready" label.

@dslomov

This comment has been minimized.

Contributor

dslomov commented Nov 20, 2018

There are still no migration docs in this issue. Please add some.

@alandonovan

This comment has been minimized.

alandonovan commented Nov 21, 2018

I'm sorry for pointing this out after you've done most of the work (but at least it hasn't been released), but I'm no longer convinced this change is an improvement. Though it makes the handling of globals and locals consistent, disallowing the first print(len) in this example,

print(len); len=1; print(len)

it creates a problem no smaller than the one it solves, yet gratuitously different from Python: the restriction was motivated by the idea that a reader shouldn't have to remember where they are in the file to know which binding of len they are referring to---in particular, they don't have to look "up". But now they have to look "down" to see whether a global declaration follows. Even though it's statically provable that the first reference to len will fail dynamically, there is no static error.

Having to look down is more surprising than having to look up; the need to look down for local variables in Python works because the function bodies fit on a page, but global variables can be thousands of lines away.

I think we should revert the spec to follow Python. (Or at the very least make the forward reference a static error too.)

@laurentlb

This comment has been minimized.

Contributor

laurentlb commented Nov 21, 2018

Having to look down is more surprising than having to look up

This can easily be reported by a linter. IDEs should be able to jump to definition too.

@alandonovan

This comment has been minimized.

alandonovan commented Nov 21, 2018

IDEs should be able to jump to definition too.

That's not relevant. The choice here is between two alternative static name resolution algorithms.

From related discussion at google/starlark-go#14 (comment):

No-one disputes the value of static name resolution; that's not the issue here.

The issue is that we can statically resolve the first len in print(len); len=1; print(len) in two different ways: the Python way, which resolves to the definition "above", that is, the predeclared one, or the Skylark-spec way, which resolves the definition "below".

The Python way is clearly useful. For example, you might want to define a local variant of a built-in (print, say), using the same name:

builtin_print = print
def print(x): ....builtin_print(x)...
...
print(...) # calls the user-defined one
There are plenty of other examples in BUILD files.

But the Skylark-spec way is literally never useful. It's a guaranteed dynamic error.

The Python way is static, inconsistent but familiar, and useful. The Skylark-spec way is static, consistent but surprising, and not useful. How is that better?

@brandjon

This comment has been minimized.

Member

brandjon commented Nov 26, 2018

I don't think it's that important that it be a static error as opposed to a linter error. Plenty of things can be determined to fail statically, such as division by literal zero.

The Python way is clearly useful. For example, you might want to define a local variant of a built-in (print, say), using the same name

It can also be mis-used as follows:

print(...)    # 1
def f(x):
    print(x)  # 2

def print(x): ...

print(...)    # 3
def g(x):
    print(x)  # 4

In Python, only (1) uses the builtin print. The use case you give could be supported by adding a builtins module to reference the original print, if we feel it's important.

bazel-io pushed a commit that referenced this issue Nov 30, 2018

Release 0.20.0 (2018-11-30)
Baseline: 7bf7f03

Cherry picks:

   + fd52341:
     update bazel-toolchains pin to latest release Part of changes to
     allow bazelci to use 0.19.0 configs. RBE toolchain configs at or
     before 0.17.0 are not compatible with bazel 0.19.0 or above.
   + 241f28d:
     Revert "Toggle --incompatible_disable_late_bound_option_defaults
     flag."
   + f7e5aef:
     Add cc_toolchain targets for the new entries in the default
     cc_toolchain_suite.
   + d2920e3:
     Revert "WindowsFileSystem: open files with delete-sharing"

[Breaking changes in 0.20](https://github.com/bazelbuild/bazel/issues?q=is%3Aissue+label%3Abreaking-change-0.20)

  - [--incompatible_remove_native_http_archive](#6570).
  - [--incompatible_remove_native_git_repository](#6569).
  - [--incompatible_disable_cc_toolchain_label_from_crosstool_proto](#6434).
  - [--incompatible_disable_depset_in_cc_user_flags](#6384).
  - [--incompatible_disable_cc_configuration_make_variables](#6381).
  - [--incompatible_disallow_conflicting_providers](#5902).
  - [--incompatible_range_type](#5264).

[0.20 is a migration window for the following changes](https://github.com/bazelbuild/bazel/issues?q=is%3Aissue+label%3Amigration-0.20)

  - [--incompatible_use_jdk10_as_host_javabase](#6661)
  - [--incompatible_use_remotejdk_as_host_javabase](#6656)
  - [--incompatible_disable_sysroot_from_configuration](#6565)
  - [--incompatible_provide_cc_toolchain_info_from_cc_toolchain_suite](#6537)
  - [--incompatible_disable_depset_in_cc_user_flags](#6383)
  - [--incompatible_package_name_is_a_function](#5827)

[Breaking changes in the next release (0.21)](https://github.com/bazelbuild/bazel/issues?q=is%3Aissue+label%3Abreaking-change-0.21)

  - [--incompatible_use_jdk10_as_host_javabase](#6661)
  - [--incompatible_use_remotejdk_as_host_javabase](#6656)
  - [--incompatible_disable_sysroot_from_configuration](#6565)
  - [--incompatible_provide_cc_toolchain_info_from_cc_toolchain_suite](#6537)
  - [--incompatible_disable_depset_in_cc_user_flags](#6383)
  - [--incompatible_disallow_data_transition](#6153)
  - [--incompatible_package_name_is_a_function](#5827)
  - [--incompatible_disallow_slash_operator](#5823)
  - [--incompatible_static_name_resolution](#5637)

Incompatible changes:

  - the --experimental_no_dotd_scanning_with_modules command line
    argument is not supported anymore.
  - The --prune_cpp_modules command line option is not supported
    anymore.
  - the --experimental_prune_cpp_input_discovery command line option
    is not supported anymore.

New features:

  - Added support for Android NDK r18.

Important changes:

  - The 'default' parameter of attr.output and attr.output_list is
    removed. This is controlled by
    --incompatible_no_output_attr_default
  - A number of platform-related Starlark APIs which were previously
    marked "experimental" are now disabled by default, and may be
    enabled via --experimental_platforms_api
  - Make legacy-test-support ("legacy_test-<api-level>") from
    android_sdk_repository neverlink. The legacy test support
    libraries shouldn't be built into test binaries. To make them
    available at runtime, developers should declare them via
    uses-library:
    https://developer.android.com/training/testing/set-up-project#andr
    oid-test-base
  - query remote server Capabilities (per REAPI v2)
  - CppRules: All cc_toolchains depended on from
    cc_toolchain_suite.toolchains are now analyzed when not using
    platforms in order to select the right cc_toolchain.
  - removed obsolete --explicit_jre_deps flag.
  - Incompatible flag
    --incompatible_disable_legacy_cpp_toolchain_skylark_api was
    flipped.
  - Improve error messaging when unsupport proguard options are
    specified at the library level.
  - Incompatible flag
    --incompatible_disable_legacy_cpp_toolchain_skylark_api was
    flipped.
  - Incompatible flag
    --incompatible_disable_legacy_cpp_toolchain_skylark_api was
    flipped.
  - The --incompatible_disable_late_bound_option_defaults flag has
    been flipped (#6384)
  - Incompatible flag
    --incompatible_disable_legacy_flags_cc_toolchain_api was flipped
    (#6434)
  - Fixed issue where ctx.resolve_command created conflicting
    intermediate files when resolve_command was called multiple times
    within the same rule invocation with a long command attribute.
  - Incompatible flag
    --incompatible_disable_cc_configuration_make_variables was
    flipped (#6381)
  - If the --javabase flag is unset, it Bazel locates a JDK using
    the JAVA_HOME environment variable and searching the PATH. If no
    JDK is found --javabase will be empty, and builds targeting Java
    will not
    be supported. Previously Bazel would fall back to using the
    embedded
    JDK as a --javabase, but this is no longer default behaviour. A
    JDK should
    be explicitly installed instead to enable Java development
  - Bazel will now shut down when idle for 5 minutes and the system
    is low on RAM (linux only).
  - CROSSTOOL file is now read from the package of cc_toolchain, not
    from
    the package of cc_toolchain_suite. This is not expected to break
    anybody since
    cc_toolchain_suite and cc_toolchain are commonly in the same
    package.
  - All overrides of Starlark's ctx.new_file function are now
    deprecated.
      Try the `--incompatible_new_actions_api` flag to ensure your
    code is forward-compatible.
  - --incompatible_disable_cc_toolchain_label_from_crosstool_proto
    was flipped.
  - Introduce --(no)shutdown_on_low_sys_mem startup flag to toggle
    idle low-memory shutdown, disabled by default.
  - --incompatible_disable_cc_toolchain_label_from_crosstool_proto
    was flipped.
  - --incompatible_disable_cc_toolchain_label_from_crosstool_proto
    was flipped.
  - CppRules: All cc_toolchains depended on from
    cc_toolchain_suite.toolchains are now analyzed when not using
    platforms in order to select the right cc_toolchain.
  - The function `attr.license` is deprecated and will be removed.
      It can be disabled now with `--incompatible_no_attr_license`.
  - `range()` function now returns a lazy value
    (`--incompatible_range_type` is now set by default).
  - The code coverage report now includes the actual paths to header
    files instead of the ugly,
    Bazel generated, virtual includes path.
  - `--incompatible_disallow_conflicting_providers` has been switched
    to true
  - Add new flag `--incompatible_disable_systool_from_configration` to
    disable loading the systool from CppConfiguration.
  - Add new flag `--incompatible_disable_sysroot_from_configuration`
    to
    disable loading the systool from CppConfiguration.
  - Sorting remote Platform properties for remote execution. May
    affect cache keys!
  - Use different server log files per Bazel server process; java.log
    is
    now a symlink to the latest log.

This release contains contributions from many people at Google, as well as a7g4 <a7g4@a7g4.net>, Alan <alan.agius@betssongroup.com>, Asaf Flescher <asafflesch@gmail.com>, Benjamin Peterson <bp@benjamin.pe>, Ed Schouten <ed.schouten@prodrive-technologies.com>, George Gensure <ggensure@uber.com>, George Kalpakas <kalpakas.g@gmail.com>, Greg <gregestren@users.noreply.github.com>, Irina Iancu <iirina@users.noreply.github.com>, Keith Smiley <keithbsmiley@gmail.com>, Loo Rong Jie <loorongjie@gmail.com>, Mark Zeren <mzeren@vmware.com>, Petros Eskinder <petroseskinder@users.noreply.github.com>, rachcatch <rachelcatchpoole@hotmail.com>, Robert Brown <robert.brown@gmail.com>, Robert Gay <robert.gay@redfin.com>, Salty Egg <2281521+zhouhao@users.noreply.github.com>.
@alandonovan

This comment has been minimized.

alandonovan commented Nov 30, 2018

Correction: I was under the misapprehension that Python's name resolution statically resolved the distinction between predeclared names and global names, but this is not the case. It treats all global names the same, and the LOAD_GLOBAL op does a dynamic lookup first in globals, then in builtins.

So, rephrasing: perhaps we should specify that name resolution statically resolves all global names to either a predeclared identifier or a global, based on the lexical environment. This is more static (and more efficient) than the Python approach, yet still allows 'print(len); len=1; print(len)' to work in the obvious way. Jon's program from the previous note would resolve uses 1 and 2 to the predeclared print, and 3 and 4 to the user-defined print. Users only need to "look up".

ahirreddy pushed a commit to ahirreddy/bazel that referenced this issue Dec 1, 2018

Release 0.20.0 (2018-11-30)
Baseline: 7bf7f03

Cherry picks:

   + fd52341:
     update bazel-toolchains pin to latest release Part of changes to
     allow bazelci to use 0.19.0 configs. RBE toolchain configs at or
     before 0.17.0 are not compatible with bazel 0.19.0 or above.
   + 241f28d:
     Revert "Toggle --incompatible_disable_late_bound_option_defaults
     flag."
   + f7e5aef:
     Add cc_toolchain targets for the new entries in the default
     cc_toolchain_suite.
   + d2920e3:
     Revert "WindowsFileSystem: open files with delete-sharing"

[Breaking changes in 0.20](https://github.com/bazelbuild/bazel/issues?q=is%3Aissue+label%3Abreaking-change-0.20)

  - [--incompatible_remove_native_http_archive](bazelbuild#6570).
  - [--incompatible_remove_native_git_repository](bazelbuild#6569).
  - [--incompatible_disable_cc_toolchain_label_from_crosstool_proto](bazelbuild#6434).
  - [--incompatible_disable_depset_in_cc_user_flags](bazelbuild#6384).
  - [--incompatible_disable_cc_configuration_make_variables](bazelbuild#6381).
  - [--incompatible_disallow_conflicting_providers](bazelbuild#5902).
  - [--incompatible_range_type](bazelbuild#5264).

[0.20 is a migration window for the following changes](https://github.com/bazelbuild/bazel/issues?q=is%3Aissue+label%3Amigration-0.20)

  - [--incompatible_use_jdk10_as_host_javabase](bazelbuild#6661)
  - [--incompatible_use_remotejdk_as_host_javabase](bazelbuild#6656)
  - [--incompatible_disable_sysroot_from_configuration](bazelbuild#6565)
  - [--incompatible_provide_cc_toolchain_info_from_cc_toolchain_suite](bazelbuild#6537)
  - [--incompatible_disable_depset_in_cc_user_flags](bazelbuild#6383)
  - [--incompatible_package_name_is_a_function](bazelbuild#5827)

[Breaking changes in the next release (0.21)](https://github.com/bazelbuild/bazel/issues?q=is%3Aissue+label%3Abreaking-change-0.21)

  - [--incompatible_use_jdk10_as_host_javabase](bazelbuild#6661)
  - [--incompatible_use_remotejdk_as_host_javabase](bazelbuild#6656)
  - [--incompatible_disable_sysroot_from_configuration](bazelbuild#6565)
  - [--incompatible_provide_cc_toolchain_info_from_cc_toolchain_suite](bazelbuild#6537)
  - [--incompatible_disable_depset_in_cc_user_flags](bazelbuild#6383)
  - [--incompatible_disallow_data_transition](bazelbuild#6153)
  - [--incompatible_package_name_is_a_function](bazelbuild#5827)
  - [--incompatible_disallow_slash_operator](bazelbuild#5823)
  - [--incompatible_static_name_resolution](bazelbuild#5637)

Incompatible changes:

  - the --experimental_no_dotd_scanning_with_modules command line
    argument is not supported anymore.
  - The --prune_cpp_modules command line option is not supported
    anymore.
  - the --experimental_prune_cpp_input_discovery command line option
    is not supported anymore.

New features:

  - Added support for Android NDK r18.

Important changes:

  - The 'default' parameter of attr.output and attr.output_list is
    removed. This is controlled by
    --incompatible_no_output_attr_default
  - A number of platform-related Starlark APIs which were previously
    marked "experimental" are now disabled by default, and may be
    enabled via --experimental_platforms_api
  - Make legacy-test-support ("legacy_test-<api-level>") from
    android_sdk_repository neverlink. The legacy test support
    libraries shouldn't be built into test binaries. To make them
    available at runtime, developers should declare them via
    uses-library:
    https://developer.android.com/training/testing/set-up-project#andr
    oid-test-base
  - query remote server Capabilities (per REAPI v2)
  - CppRules: All cc_toolchains depended on from
    cc_toolchain_suite.toolchains are now analyzed when not using
    platforms in order to select the right cc_toolchain.
  - removed obsolete --explicit_jre_deps flag.
  - Incompatible flag
    --incompatible_disable_legacy_cpp_toolchain_skylark_api was
    flipped.
  - Improve error messaging when unsupport proguard options are
    specified at the library level.
  - Incompatible flag
    --incompatible_disable_legacy_cpp_toolchain_skylark_api was
    flipped.
  - Incompatible flag
    --incompatible_disable_legacy_cpp_toolchain_skylark_api was
    flipped.
  - The --incompatible_disable_late_bound_option_defaults flag has
    been flipped (bazelbuild#6384)
  - Incompatible flag
    --incompatible_disable_legacy_flags_cc_toolchain_api was flipped
    (bazelbuild#6434)
  - Fixed issue where ctx.resolve_command created conflicting
    intermediate files when resolve_command was called multiple times
    within the same rule invocation with a long command attribute.
  - Incompatible flag
    --incompatible_disable_cc_configuration_make_variables was
    flipped (bazelbuild#6381)
  - If the --javabase flag is unset, it Bazel locates a JDK using
    the JAVA_HOME environment variable and searching the PATH. If no
    JDK is found --javabase will be empty, and builds targeting Java
    will not
    be supported. Previously Bazel would fall back to using the
    embedded
    JDK as a --javabase, but this is no longer default behaviour. A
    JDK should
    be explicitly installed instead to enable Java development
  - Bazel will now shut down when idle for 5 minutes and the system
    is low on RAM (linux only).
  - CROSSTOOL file is now read from the package of cc_toolchain, not
    from
    the package of cc_toolchain_suite. This is not expected to break
    anybody since
    cc_toolchain_suite and cc_toolchain are commonly in the same
    package.
  - All overrides of Starlark's ctx.new_file function are now
    deprecated.
      Try the `--incompatible_new_actions_api` flag to ensure your
    code is forward-compatible.
  - --incompatible_disable_cc_toolchain_label_from_crosstool_proto
    was flipped.
  - Introduce --(no)shutdown_on_low_sys_mem startup flag to toggle
    idle low-memory shutdown, disabled by default.
  - --incompatible_disable_cc_toolchain_label_from_crosstool_proto
    was flipped.
  - --incompatible_disable_cc_toolchain_label_from_crosstool_proto
    was flipped.
  - CppRules: All cc_toolchains depended on from
    cc_toolchain_suite.toolchains are now analyzed when not using
    platforms in order to select the right cc_toolchain.
  - The function `attr.license` is deprecated and will be removed.
      It can be disabled now with `--incompatible_no_attr_license`.
  - `range()` function now returns a lazy value
    (`--incompatible_range_type` is now set by default).
  - The code coverage report now includes the actual paths to header
    files instead of the ugly,
    Bazel generated, virtual includes path.
  - `--incompatible_disallow_conflicting_providers` has been switched
    to true
  - Add new flag `--incompatible_disable_systool_from_configration` to
    disable loading the systool from CppConfiguration.
  - Add new flag `--incompatible_disable_sysroot_from_configuration`
    to
    disable loading the systool from CppConfiguration.
  - Sorting remote Platform properties for remote execution. May
    affect cache keys!
  - Use different server log files per Bazel server process; java.log
    is
    now a symlink to the latest log.

This release contains contributions from many people at Google, as well as a7g4 <a7g4@a7g4.net>, Alan <alan.agius@betssongroup.com>, Asaf Flescher <asafflesch@gmail.com>, Benjamin Peterson <bp@benjamin.pe>, Ed Schouten <ed.schouten@prodrive-technologies.com>, George Gensure <ggensure@uber.com>, George Kalpakas <kalpakas.g@gmail.com>, Greg <gregestren@users.noreply.github.com>, Irina Iancu <iirina@users.noreply.github.com>, Keith Smiley <keithbsmiley@gmail.com>, Loo Rong Jie <loorongjie@gmail.com>, Mark Zeren <mzeren@vmware.com>, Petros Eskinder <petroseskinder@users.noreply.github.com>, rachcatch <rachelcatchpoole@hotmail.com>, Robert Brown <robert.brown@gmail.com>, Robert Gay <robert.gay@redfin.com>, Salty Egg <2281521+zhouhao@users.noreply.github.com>.

bazel-io pushed a commit that referenced this issue Dec 5, 2018

Code simplification in the environment, remove old incompatible flags
Remove --incompatible_static_name_resolution and --incompatible_package_name_is_a_function

#5827
#5637

RELNOTES: None.
PiperOrigin-RevId: 224140026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment