This stems from discussion on Basic syntax #162:
https://github.com/carbon-language/carbon-lang/pull/162/files#r513840204

To summarize: should there be a function type, and if so, what form should it take? tituswinters highlights concerns about overload sets.

I think that key questions are:

• Do we want a function type that can point at a specific function signature? (if not, does that moot this discussion?)
• Do we want an introducer? If so, fn, fnty, or something else?
• Should the return type be required? That is, if -> return_type isn't present, is that a syntax error or does it indicate that an empty tuple is being returned?

This OSS Github plugin appears to “just work” (I tested it) and is super-easy to set up. We should try it.

https://github.com/marketplace/dpulls

At the 2020-09-22 core team meeting, it was decided that the open question in the "Change markdown style guide" proposal of which style guide to use would be left to the Painter. This issue is to track that decision.

Proposal #143 suggests that we require a period in all real number literals. This means that constants such as 1e6 are invalid -- they're neither real number literals nor integer literals. The motivation for rejecting these in particular is that a survey of C++ code found that they were mostly used to express integer literals rather than real number literals (eg, int kMaxWidgets = 1e6;).

We should consider accepting such cases as integer literals once Carbon is more established and the divergence from C and C++ is a less significant concern.

It appears to be passing 0 as a line number

Unfortunately, I have a state that only crashes on my mac, and I'm not getting any symbols, sadly.

$ bazel run -c dbg //executable_semantics --run_under=lldb
...
INFO: Running command line: /bin/bash -c 'lldb bazel-bin/executable_semantics/executable_semantics '
INFO: Build completed successfully, 174 total actions
(lldb) target create "/private/var/tmp/_bazel_dabrahams/7eb59300bade26410e9aa17cfd0b52e9/execroot/carbon/bazel-out/darwin-dbg/bin/executable_semantics/executable_semantics"
Current executable set to '/private/var/tmp/_bazel_dabrahams/7eb59300bade26410e9aa17cfd0b52e9/execroot/carbon/bazel-out/darwin-dbg/bin/executable_semantics/executable_semantics' (x86_64).
(lldb) process launch -- /Users/dabrahams/src/carbon-lang/executable_semantics/testdata/undef1.6c
process launch -- /Users/dabrahams/src/carbon-lang/executable_semantics/testdata/undef1.6c
Process 6982 launched: '/private/var/tmp/_bazel_dabrahams/7eb59300bade26410e9aa17cfd0b52e9/execroot/carbon/bazel-out/darwin-dbg/bin/executable_semantics/executable_semantics' (x86_64)
Process 6982 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x00007fff2067e5d2 libsystem_platform.dylib`_platform_strlen + 18
libsystem_platform.dylib`_platform_strlen:
->  0x7fff2067e5d2 <+18>: pcmpeqb (%rdi), %xmm0
    0x7fff2067e5d6 <+22>: pmovmskb %xmm0, %esi
    0x7fff2067e5da <+26>: andq   $0xf, %rcx
    0x7fff2067e5de <+30>: orq    $-0x1, %rax
Target 0: (executable_semantics) stopped.
(lldb) up 3

frame #3: 0x00000001000158e2 executable_semantics`Carbon::PrintSyntaxError(char*, int) + 34
executable_semantics`Carbon::PrintSyntaxError:
->  0x1000158e2 <+34>: movq   %rax, %rdi
    0x1000158e5 <+37>: leaq   0x15bd8b(%rip), %rsi      ; "':'"
    0x1000158ec <+44>: callq  0x1001690fc               ; symbol stub for: std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::operator<<<std::__1::char_traits<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, char const*)
    0x1000158f1 <+49>: movq   %rax, %rdi
(lldb) up

frame #4: 0x000000010000bec6 executable_semantics`yyerror(std::__1::optional<std::__1::list<Carbon::Declaration, std::__1::allocator<Carbon::Declaration> >*> const&, Carbon::SyntaxDriver&, char*) + 38
executable_semantics`yyerror:
->  0x10000bec6 <+38>: addq   $0x20, %rsp
    0x10000beca <+42>: popq   %rbp
    0x10000becb <+43>: retq   
    0x10000becc <+44>: nopl   (%rax)
(lldb) 

In the type checker, rename env to types and ct_env to values.
In the interpreter, rename env to values.

Building on Windows is low priority but it may be helpful to have a common place where we keep track of known problems.

The first problem I hit was https://github.com/google/llvm-bazel fails if it can't create symlinks, which Windows doesn't allow by default for unprivileged users. This can be changed by enabling Developer Mode.

Current blocker is our toolchain configuration expects to find Clang at bin/clang but it's bin/clang.exe. Once that's fixed I assume there will be wrinkles around turning clang.exe into clang++.exe.

It surprised me when I found a bunch of my posts had been rejected because it won't receive email replies. It's easy to set this up and will be convenient for lots of folks.

(Prior discussion: #83 (comment))

In docs/design/README.md, I think the section titled "Carbon <-> C/C++ interoperability" should be renamed to something like "Bidirectional C++ interoperability": I think it puts more emphasis on the bidirectionality, without requiring the "<" HTML entity (or untypable "↔" character, as with prior drafts"), and without the redundant mention of "Carbon". The same applies to the title of docs/design/interoperability/template.md.

The if2.6c example segfaults. I'm currently in the process of temporarily disabling it and making our build not hide the problem. #312 will disable the test by commenting it out of the BUILD file.

There are still several direct error reporting bits of code that need to be updated to the diagnostics library now that we have one (even with its somewhat minimal API currently).

This requires adding the --report=state flag and adding parser.output to the outs.

genrule(
    name = "syntax_bison_srcs",
    srcs = ["parser.ypp"],
    outs = [
        "parser.cpp",
        "parser.h",
        "parser.output",
    ],
    cmd = "M4=$(M4) $(BISON) " +
          "--output=$(location parser.cpp) " +
          "--report=state " +
          "--defines=$(location parser.h) " +
          "$(location parser.ypp)",
    toolchains = [
        "@rules_bison//bison:current_bison_toolchain",
        "@rules_m4//m4:current_m4_toolchain",
    ],
)

proposals/p0143.md contains the following table:

| Base specifier | Base | Digits                   |
| -------------- | ---- | ------------------------ |
| `b`            | 2    | `0` and `1`              |
| `x`            | 16   | `0` ... `9`, `A` ... `F` |

In the GitHub preview, this renders as a table, as intended, but on https://carbon-lang.dev/proposals/p0143.html it becomes

<p>| Base specifier | Base | Digits                   |
| -------------- | ---- | ------------------------ |
| <code>b</code>            | 2    | <code>0</code> and <code>1</code>              |
| <code>x</code>            | 16   | <code>0</code> ... <code>9</code>, <code>A</code> ... <code>F</code> |</p>

Which renders as something like "| Base specifier | Base | Digits | | -------------- | ---- | ------------------------ | | b | 2 | 0 and 1 | | x | 16 | 0 ... 9, A ... F |"

As we refactor the interpreter, we need to make sure that we don't accidentally box ourselves in with
and approach that can't handle the control-flow features that we may want to add in the future, such as
exceptions, question mark, coroutines, threads, etc.

So the idea is to demo a fairly powerful control-flow feature now as a place-holder, such as first-class continuations (or perhaps delimited continuations), even though we may not (probably do not?) want this feature in Carbon.

proposals/p0143.md contains the following line:

`0x123.p456` (= 123<sub>16</sub> \* 2<sup>456</sup>)

As currently published at https://carbon-lang.dev/proposals/p0143.html, that line appears as:

<code>0x123.p456</code> (= 123<!-- raw HTML omitted -->16<!-- raw HTML omitted --> * 2<!-- raw HTML omitted -->456<!-- raw HTML omitted -->)

Which renders in the browser as "0x123.p456 (= 12316 * 2456)" instead of the intended "0x123.p456 (= 123₁₆ * 2⁴⁵⁶)"

The "test (ubuntu-latest, opt)" and "test (ubuntu-latest, fastbuild)" checks for PR #417 are failing with errors like the following:

Run bazelisk --version
2021/03/25 19:53:14 Using unreleased version at commit 1d20ea76113e49f7fb607ef92231f3665797b8e6
2021/03/25 19:53:14 Downloading https://storage.googleapis.com/bazel-builds/artifacts/ubuntu1404/1d20ea76113e49f7fb607ef92231f3665797b8e6/bazel...
2021/03/25 19:53:14 could not download Bazel: HTTP GET https://storage.googleapis.com/bazel-builds/artifacts/ubuntu1404/1d20ea76113e49f7fb607ef92231f3665797b8e6/bazel failed with error 404
Error: Process completed with exit code 1.

This doesn't have any plausible relation to the content of the PR, so I think it's an infrastructure bug.

Per @jsiek, many names are abbreviated; spelling them out would improve comprehensibility a lot.

#237 (particularly executable_semantics/BUILD) relies on a system-installed bison/flex. It'd be nice to have these built via bazel.

Note, I'm aware of https://github.com/jmillikin/rules_bison and https://github.com/jmillikin/rules_flex; however, these print build warnings with Carbon's toolchain, and I think those build warnings make a bad experience. One option would be to contribute changes to those to disable the necessary warnings (without an explanation referencing Carbon in a leaking way), another would be to build something hermetic from scratch.

update PrintEnv and PrintTypeEnv to use the iterators

one suggestion: BindPatternVariables

There's also a suggestion to make PatternMatch a method of Dictionary.
But PatternMatch is primarily a switch statement over values... which
suggests that it should be a method of the Value class.

In case this hasn't already been done, the GH repo should be configured so that CI must succeed before anything can be merged to trunk.

On macOS, at 9ed1c5c, with or without .bazeliskrc wired to 4.0.0:

$ bazelisk build //executable_semantics:executable_semantics.stripped
[...]
ERROR: /Users/mattdr/src/carbon-lang/executable_semantics/BUILD:11:10: Stripping executable_semantics/executable_semantics.stripped for //executable_semantics:executable_semantics failed: (Exit 1): llvm-strip failed: error executing command /private/var/tmp/_bazel_mattdr/d9970d429502ef17c1da219b3a765faf/external/bootstrap_clang_toolchain/bin/llvm-strip

Use --sandbox_debug to see verbose messages from the sandbox llvm-strip failed: error executing command /private/var/tmp/_bazel_mattdr/d9970d429502ef17c1da219b3a765faf/external/bootstrap_clang_toolchain/bin/llvm-strip

Use --sandbox_debug to see verbose messages from the sandbox
OVERVIEW: llvm-strip tool

USAGE: llvm-strip [options] inputs...
[...]

We can confirm with aquery that Bazel is invoking llvm-strip with no arguments:

$ bazelisk aquery 'mnemonic("CcStrip", //executable_semantics:executable_semantics)'
INFO: Analyzed target //executable_semantics:executable_semantics (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
action 'Stripping executable_semantics/executable_semantics.stripped for //executable_semantics:executable_semantics'
  Mnemonic: CcStrip
  Target: //executable_semantics:executable_semantics
  Configuration: darwin-fastbuild
  ActionKey: f5e061b91160a2d1693c98ea4144f0090598e0d626416aa402d52095236980ed
  Inputs: [bazel-out/darwin-fastbuild/bin/executable_semantics/executable_semantics, bazel-out/darwin-opt-exec-2B5CBBC6/internal/_middlemen/external_Sbazel_Ucc_Utoolchain_Cempty]
  Outputs: [bazel-out/darwin-fastbuild/bin/executable_semantics/executable_semantics.stripped]
  Environment: [LANG=en_US.UTF-8]
  Command Line: (exec /private/var/tmp/_bazel_mattdr/d9970d429502ef17c1da219b3a765faf/external/bootstrap_clang_toolchain/bin/llvm-strip)

By comparison, if I create another Bazel workspace elsewhere and let it pick up the local toolchain, we see:

$ bazelisk aquery 'mnemonic("CcStrip", //:hello)'
INFO: Analyzed target //:hello (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
action 'Stripping hello.stripped for //:hello'
  Mnemonic: CcStrip
  Target: //:hello
  Configuration: darwin-fastbuild
  ActionKey: 010f253cf13d63c4f2509c10f0ee0b534d93c03d0c65d24a4398b1c6e04d6068
  Inputs: [bazel-out/darwin-fastbuild/bin/hello, bazel-out/darwin-opt-exec-2B5CBBC6/internal/_middlemen/external_Slocal_Uconfig_Ucc_Cempty]
  Outputs: [bazel-out/darwin-fastbuild/bin/hello.stripped]
  Command Line: (exec /usr/bin/strip \
    -S \
    -o \
    bazel-out/darwin-fastbuild/bin/hello.stripped \
    bazel-out/darwin-fastbuild/bin/hello)

that configuration appears to come from:
https://github.com/bazelbuild/bazel/blob/073ae810e4f61f56f68a31d5ee95f18633d176cc/tools/osx/crosstool/cc_toolchain_config.bzl#L296-L311

meanwhile, we configure the strip action but don't attach any flag_sets, so I guess Bazel just thinks it can run it with no flags.

Example failure:

github_tools/pr_comments.py  --comments-after zygoloid 273
Loading https://github.com/carbon-language/carbon-lang/pull/273 ...Traceback (most recent call last):
  File "github_tools/pr_comments.py", line 457, in <module>
    main()
  File "github_tools/pr_comments.py", line 438, in main
    comments, threads_by_path = _fetch_comments(parsed_args)
  File "github_tools/pr_comments.py", line 390, in _fetch_comments
    client = github_helpers.Client(parsed_args)
  File "github_tools/github_helpers.py", line 51, in __init__
    self._client = gql.Client(
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/gql/client.py", line 42, in __init__
    schema = build_client_schema(introspection)
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/utils/build_client_schema.py", line 310, in build_client_schema
    return GraphQLSchema(
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/schema.py", line 106, in __init__
    self._type_map = GraphQLTypeMap(initial_types)
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 31, in __init__
    self.update(reduce(self.reducer, types, OrderedDict()))  # type: ignore
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 142, in reducer
    reduced_map = cls.reducer(reduced_map, getattr(field, "type", None))
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 142, in reducer
    reduced_map = cls.reducer(reduced_map, getattr(field, "type", None))
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 85, in reducer
    return cls.reducer(map_, type_.of_type)
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 142, in reducer
    reduced_map = cls.reducer(reduced_map, getattr(field, "type", None))
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 85, in reducer
    return cls.reducer(map_, type_.of_type)
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 142, in reducer
    reduced_map = cls.reducer(reduced_map, getattr(field, "type", None))
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 100, in reducer
    reduced_map = cls.reducer(reduced_map, t)
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 142, in reducer
    reduced_map = cls.reducer(reduced_map, getattr(field, "type", None))
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 85, in reducer
    return cls.reducer(map_, type_.of_type)
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 142, in reducer
    reduced_map = cls.reducer(reduced_map, getattr(field, "type", None))
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 85, in reducer
    return cls.reducer(map_, type_.of_type)
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 142, in reducer
    reduced_map = cls.reducer(reduced_map, getattr(field, "type", None))
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/typemap.py", line 109, in reducer
    field_map = type_.fields
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/pyutils/cached_property.py", line 22, in __get__
    value = obj.__dict__[self.func.__name__] = self.func(obj)
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/definition.py", line 198, in fields
    return define_field_map(self, self._fields)
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/type/definition.py", line 212, in define_field_map
    field_map = field_map()
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/utils/build_client_schema.py", line 163, in <lambda>
    fields=lambda: build_field_def_map(object_introspection),
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/utils/build_client_schema.py", line 224, in build_field_def_map
    [
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/utils/build_client_schema.py", line 232, in <listcomp>
    args=build_input_value_def_map(f.get("args"), GraphQLArgument),
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/utils/build_client_schema.py", line 248, in build_input_value_def_map
    [
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/utils/build_client_schema.py", line 249, in <listcomp>
    (f["name"], build_input_value(f, argument_type))
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/utils/build_client_schema.py", line 258, in build_input_value
    default_value=build_default_value(input_value_introspection),
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/utils/build_client_schema.py", line 244, in build_default_value
    return value_from_ast(parse_value(default_value), get_input_type(f["type"]))
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/language/parser.py", line 76, in parse_value
    return parse_value_literal(parser, False)
  File "/Users/richardsmith/.pyenv/versions/3.8.6/lib/python3.8/site-packages/graphql/language/parser.py", line 512, in parse_value_literal
    raise unexpected(parser)
graphql.error.syntax_error.GraphQLSyntaxError: Syntax Error GraphQL (1:1) Unexpected Name "null"

1: null
   ^

This seems to happen for all PRs. (Tested 198, 199, 273.) Doesn't look like anything's changed in the script.

Here's the input value that it's processing in build_input_value:

{'name': 'isVerified', 'description': 'Filter by if the domain is verified.', 'type': {'kind': 'SCALAR', 'name': 'Boolean', 'ofType': None}, 'defaultValue': 'null'}

It was suggested in the 2020-07-01 Carbon meeting that there should be a glossary of term that are used when discussing Carbon to ensure that all parties has a common understanding.

This stems from discussion about Basic syntax #162. While there is interest in having types be values, the core team would like to be clear that it's an undecided issue that needs more investigation.

According to the grammar in the basic syntax proposal (#162) it seems like curried function types require repeating the fnty keyword:

expression:  "fnty" tuple return_type
return_type:  "->" expression

For example:

fnty (Int) -> Int // OK
fnty (Int) -> fnty (Int) -> Int // repeated fnty
fnty (Int) -> (Int) -> Int // desired

I understand why it happens, and I see that it is uniform with regards to the expression production, but the result -- repeated fnty keywords -- does not seem aesthetically pleasing to me.

Currently the function is defined in syntax/.

Principle: Write Building Codes, Not Style Guides

Table of contents

• Background
• Principle
• Applications of these principles

Background

Most languages come with a set of rules around naming,
use of whitespace, character sets, and the like. These are enforced by the
compiler and other tools. Some of these rules are for ease of parsing or compiling, while others are to ensure the code is readable and maintainable. In addition,
most languages grow an ecosystem of style guides that recommend
various best practices around naming, whitespace, character sets,
length of names, length of functions, and so on. These guides compete
for followers and adherents. For a new language, there is pressure to
include more rules in the language, so as to need less of them in the guides, and to have more consistency.

Principle

The principle here relies on an analogy: in creating a house, you might well
consult any number of style guides for advice on what colour to paint things,
whether all the bedroms should be on the same floor, or what size a
balcony needs to be before anyone actually uses it. But your house must
conform to a number of building codes as well: rules about the spacing
of the stair railings, the thickness of support beams, and the presence
of windows in bedrooms for emergency exit. These are safety rules, not a
matter of taste.

Carbon should have a building code. These are enforced rules designed
to prevent bugs and exploits. Carbon doesn't need a style guide and
can't really choose among various competing preferences for tabstops,
naming conventions, or whether or not comments need to be full sentences.

Applications of these principles

Carbon could insist that local variable names use a particular casing
convention, that lines be a maximum length, or that a particular
indentation strategy be followed. If the motivation for such a rule
is "I think it will make the code easier to read" that is a style guide,
and we should not encode it into the language. Compare those
to a rule such as "braces are not optional for if and while constructs."
If the motivation for that rule is to prevent GOTO FAIL, it is more
of a building code rule than a style guide rule.

Ideally we would encode no style guides at all, and our building
code would be rock solid, preventing all the kinds of bugs a language
can prevent through language design (leaving plenty of room for bugs
a language can't prevent.)

Consider primitives in LLVM.

One goal is to provide enough primitives to be able to build libraries for common
things like strings, containers, etc.

Another goal is to be able to implement woff2.

I've mistakenly created upstream branches for PRs, which I'm allowed to do by the system, but I don't have permission to delete the branches (or push any new commits to them). I'm really sorry to create work for other people, but I'm unable to fix the problem I've created by myself.

Seems like possibly @zygoloid has made the same mistake: https://github.com/carbon-language/carbon-lang/branches/all

So there are two issues here: one is the extra branches, the other is the mismatch between stated policy and enforced policy.

Note: I suggest letting those PRs play out even though the branches are in the wrong place, and just fixing the second issue.

Note: this is not really a problem. If you have the time to explain why my attempt to point at bison-3.7.5 didn't work, that would be great. Otherwise feel free to close.

The executable semantics code is full of pointers and low-level haxx in part because nobody knew about
https://www.gnu.org/software/bison/manual/html_node/C_002b_002b-Bison-Interface.html or https://www.gnu.org/software/bison/manual/html_node/C_002b_002b-Variants.html

But using those requires a much newer version of Bison than v3.3.x that is supported by the existing rules_bisonActually v3.3.2 has the necessary features. I was seeing errors for other reasons
I attempted to make my own update that included the new version but bazel merrily kept using the earlier version.

See chandlerc/rules_bison@master...dabrahams:bison-3.7.5
and https://github.com/dabrahams/carbon-lang/compare/existential-handroll...dabrahams:modern-bison?expand=1

Perhaps @chandlerc can help
/cc @jsiek

Just tracking a few changes to the goals #51 requested by @josh11b

(not making these up-front due to decision in progress)

Per @jsiek, we can unwind some nesting, eliminate use-once variables, and clarify by using methods where appropriate.

The message looks like this:

DEBUG: /private/var/tmp/_bazel_dabrahams/7eb59300bade26410e9aa17cfd0b52e9/external/rules_foreign_cc/workspace_definitions.bzl:6:6: `@rules_foreign_cc//:workspace_definitions.bzl` has been replaced by `@rules_foreign_cc//foreign_cc:repositories.bzl`. Please use the updated source location

Per @jsiek, there are a number of places where a notional mutation is being expressed monadically. It would improve comprehensibility to actually use idiomatic mutation of C++ value types.

Those were both accidentally included. They both use pointers which are not present in the executable semantics.

The term "execution" implies runtime, but these two tables are for type checking at compile time.
Also, using std::pair hurts the readability of the code because it makes it more difficult
to understand the different roles of the two different symbol tables. The first maps names of runtime entities to their declared types. The second maps names of compile-time entities to their values.

Per @jsiek. It will help readability to see the words stack, push, and pop in the code where that's the actual abstraction at work.

Background

Today we use a CODEOWNERS file to mete out responsibility for different parts of the tree to different "teams", e.g. implementation-team.

This is not without problems. At least once so far, we've needed an admin to "break in" to a team in order to approve a change when other team members weren't available.

Worse, reviews that match a CODEOWNERS line are immutably assigned to the responsible team. The PR notifies every member of the team. When teams have many people and are "responsible" for large sections of the codebase -- which describes implementation-team today -- this means notifications for a lot of unrelated PRs.

This had bad effects for authors and reviewers: reviewers are spammed with notifications, and authors' PRs go unreviewed because it's not obvious who from the large team should pick them up. (Elsewhere this latter effect was described as "if everyone is responsible, no one is".)

The notification e-mails sent by GitHub for "on behalf of team" PR reviews do not differ from directly-assigned reviews in a way that can be easily filtered.

The only flexibility available at the GitHub level is to configure code review assignment to replace the team with a round-robin or load-balanced selection of one team member. This has at least two downsides:

• The team member is chosen essentially at random, while the author may have a specific reviewer in mind with context
• It's not obvious that another team member can step in and review. (Requires testing.)

This has been a customer complaint for CODEOWNERS since at least April 2018 with no promising response from GitHub.

Some projects have built their own solutions:

• Bionic uses CODEOWNERS but assigns reviews to bots that in turn delegate to team members.
• Automattic wrote their own GitHub action to enforce required reviews from team members as a post-hoc check with no team assignment
• Kubernetes uses an entirely bespoke OWNERS infrastructure: https://go.k8s.io/owners

Recommendation

For Carbon, I suggest that we dispense entirely with CODEOWNERS for now. Instead, we should just use GitHub's existing ability to require one review from any committer with write access. We can maintain CODEOWNERS or OWNERS style files to make it easy for authors to discover reviewers.

Our set of contributors is small enough that we can employ a high-trust system where reviewers are relied on to stick to their area and bring in other reviewers as necessary. This is comparable to how LLVM's code review works. Reviews are public and auditable so we can follow up in the unlikely case a reviewer oversteps their bounds.

If and when this becomes unruly, we can look at building a bot or GitHub action or other automation to enforce required reviews without clogging notifications or enforcing rigidity of roles.

We should decide to what extent and in what ways we want to mitigate the risk of underhanded Carbon code -- code that intentionally behaves in a manner different from its appearance.

See https://github.com/carbon-language/carbon-lang/pull/17/files#r428333473 for a case where this question arose.

For example, replacing

    std::optional<Address> a = scope->env.Get(l);
    if (!a) {
      std::cerr << "internal error in KillScope" << std::endl;
      exit(-1);
    }

with
auto stackFrame = *scope->env.Get(l).valueor(die("internal error in Killscope"));

It's not great to look through the proposals and not have the resolution of the proposal visible in the document. The Swift process with revision numbers (for things that went through multiple reviews) and references to discussion threads is easier for me (example here: https://github.com/apple/swift-evolution/blob/main/proposals/0244-opaque-result-types.md). Can we consider a format like that?

Both the lexer and parser libraries use a similar technique for testing. They define structs suitable for use with designated initializers, and then hand them to matchers.

It might be worthwhile to switch to building matchers directly instead, as this would likely produce nicer error messages. One challenge will be test readability, but there may be ways to address this.

For tokens, this seems fairly straight forward, but for the parser this is more interesting.

Filing this issue just to track the overall discussion around the best design here, and implementing whatever the result looks like.