api file default public (#752)

Replace library public-like `api` behavior with explicit `private` behavior, mirroring #665 

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>

docs/design/code_and_name_organization/README.md

@@ -339,28 +339,27 @@ for separate compilation.
 
 #### Exporting entities from an API file
 
-In order to actually be part of a library's API, entities must both be in the
-API file and explicitly marked as an API. This is done using the `api` keyword,
-which is only allowed in the API file. For example:
+Entities in the API file are part of the library's public API by default. They
+may be marked as `private` to indicate they should only be visible to other
+parts of the library.
 
 ```carbon
 package Geometry library "Shapes" api;
 
-// Circle is marked as an API, and will be available to other libraries as
-// Geometry.Circle.
-api struct Circle { ... }
+// Circle is an API, and will be available to other libraries as
+ Geometry.Circle.
+struct Circle { ... }
 
-// CircleHelper is not marked as an API, and so will not be available to other
-// libraries.
-fn CircleHelper(Circle circle) { ... }
+// CircleHelper is private, and so will not be available to other libraries.
+private fn CircleHelper(Circle circle) { ... }
 
 // Only entities in namespaces should be marked as an API, not the namespace
 // itself.
 namespace Operations;
 
-// Operations.GetCircumference is marked as an API, and will be available to
+// Operations.GetCircumference is an API, and will be available to
 // other libraries as Geometry.Operations.GetCircumference.
-api fn Operations.GetCircumference(Circle circle) { ... }
+fn Operations.GetCircumference(Circle circle) { ... }
 ```
 
 This means that an API file can contain all implementation code for a library.
@@ -374,7 +373,9 @@ However, separate implementation files are still desirable for a few reasons:
 -   From a code maintenance perspective, having smaller files can make a library
     more maintainable.
 
-Use of the `api` keyword is not allowed within files marked as `impl`.
+Entities in the `impl` file should never have visibility keywords. If they are
+forward declared in the `api` file, they use the declaration's visibility; if
+they are only present in the `impl` file, they are implicitly `private`.
 
 #### Granularity of libraries
 
@@ -404,7 +405,7 @@ package Checksums library "Sha" api;
 
 namespaces Sha256;
 
-api fn Sha256.HexDigest(Bytes data) -> String { ... }
+fn Sha256.HexDigest(Bytes data) -> String { ... }
 ```
 
 Calling code may look like:
@@ -434,7 +435,7 @@ import IDENTIFIER (library NAME_PATH)?;
 ```
 
 An import declares a package entity named after the imported package, and makes
-`api`-tagged entities from the imported library through it. The full name path
+API entities from the imported library available through it. The full name path
 is a concatenation of the names of the package entity, any namespace entities
 applied, and the final entity addressed. Child namespaces or entities may be
 [aliased](/docs/design/aliases.md) if desired.
@@ -444,7 +445,7 @@ For example, given a library:
 ```carbon
 package Math api;
 namespace Trigonometry;
-api fn Trigonometry.Sin(...);
+fn Trigonometry.Sin(...);
 ```
 
 Calling code would import it and use it like:
@@ -574,12 +575,12 @@ server for open source packages. Conflicts can also be addressed by renaming one
 of the packages, either at the source, or as a local modification.
 
 We do need to address the case of package names conflicting with other entity
-names. It's possible that a pre-existing `api` entity will conflict with a new
-import, and that the `api` is infeasible to rename due to existing callers.
-Alternately, the `api` entity may be using an idiomatic name that it would
-contradict naming conventions to rename. In either case, this conflict may exist
-in a single file without otherwise affecting users of the API. This will be
-addressed by [name lookup](/docs/design/name_lookup.md).
+names. It's possible that a pre-existing entity will conflict with a new import,
+and that renaming the entity is infeasible to rename due to existing callers.
+Alternately, the entity may be using an idiomatic name that it would contradict
+naming conventions to rename. In either case, this conflict may exist in a
+single file without otherwise affecting users of the API. This will be addressed
+by [name lookup](/docs/design/name_lookup.md).
 
 ### Potential refactorings
 
@@ -816,10 +817,13 @@ should be part of a larger testing plan.
     -   [Collapse file and library concepts](/proposals/p0107.md#collapse-file-and-library-concepts)
     -   [Collapse the library concept into packages](/proposals/p0107.md#collapse-the-library-concept-into-packages)
     -   [Collapse the package concept into libraries](/proposals/p0107.md#collapse-the-package-concept-into-libraries)
+    -   [Default `api` to private](/proposals/p0752.md#default-api-to-private)
+    -   [Default `impl` to public](/proposals/p0752.md#default-impl-to-public)
     -   [Different file type labels](/proposals/p0107.md#different-file-type-labels)
     -   [Function-like syntax](/proposals/p0107.md#function-like-syntax)
     -   [Inlining from implementation files](/proposals/p0107.md#inlining-from-implementation-files)
     -   [Library-private access controls](/proposals/p0107.md#library-private-access-controls)
+    -   [Make keywords either optional or required in separate definitions](/proposals/p0752.md#make-keywords-either-optional-or-required-in-separate-definitions)
     -   [Managing API versus implementation in libraries](/proposals/p0107.md#managing-api-versus-implementation-in-libraries)
     -   [Multiple API files](/proposals/p0107.md#multiple-api-files)
     -   [Name paths as library names](/proposals/p0107.md#name-paths-as-library-names)

proposals/README.md

@@ -69,6 +69,7 @@ request:
 -   [0680 - And, or, not](p0680.md)
 -   [0722 - Nominal classes and methods](p0722.md)
 -   [0731 - Generics details 2: adapters, associated types, parameterized interfaces](p0731.md)
+-   [0752 - `api` file default-`public`](p0752.md)
 -   [0777 - Inheritance](p0777.md)
 
 <!-- endproposals -->

proposals/p0752.md

@@ -0,0 +1,191 @@
+# `api` file default-`public`
+
+<!--
+Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+Exceptions. See /LICENSE for license information.
+SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+-->
+
+[Pull request](https://github.com/carbon-language/carbon-lang/pull/752)
+
+<!-- toc -->
+
+## Table of contents
+
+-   [Problem](#problem)
+-   [Background](#background)
+-   [Proposal](#proposal)
+-   [Rationale based on Carbon's goals](#rationale-based-on-carbons-goals)
+-   [Alternatives considered](#alternatives-considered)
+    -   [Default `api` to private](#default-api-to-private)
+    -   [Default `impl` to public](#default-impl-to-public)
+    -   [Make keywords either optional or required in separate definitions](#make-keywords-either-optional-or-required-in-separate-definitions)
+
+<!-- tocstop -->
+
+## Problem
+
+Question for leads
+[#665: private vs public _syntax_ strategy, as well as other visibility tools like external/api/etc.](https://github.com/carbon-language/carbon-lang/issues/665)
+decided that methods on classes should default to public. Should `api` echo the
+similar strategy?
+
+## Background
+
+-   In C++, `struct` members default public, while `class` members default
+    `public`.
+-   In proposal
+    [#107: Code and name organization](https://github.com/carbon-language/carbon-lang/pull/107),
+    an `api` keyword was used to indicate public APIs within an `api` file.
+-   In [#665](https://github.com/carbon-language/carbon-lang/issues/665), it was
+    decided that Carbon class members should default `public`.
+    -   This issue was reopened to discuss alternatives in this proposal.
+
+## Proposal
+
+APIs in the `api` file should default public, without need for an additional
+`api` keyword. `private` may be specified to designate APIs that are internal to
+the library, and only visible to `impl` files.
+
+Nothing is necessary within `impl` files, and APIs there will be private unless
+forward declared in the `api` file.
+
+## Rationale based on Carbon's goals
+
+-   [Code that is easy to read, understand, and write](/docs/project/goals.md#code-that-is-easy-to-read-understand-and-write):
+    It will be easier for developers to understand code if APIs have
+    semantically similar behavior when comparing the visibility of class methods
+    to other code, and the library to other packages.
+
+## Alternatives considered
+
+### Default `api` to private
+
+Default private is what was implied by `api`, and was the previous state.
+
+Advantages:
+
+-   Decreases the likelihood that developers will accidentally expose APIs,
+    because it's an explicit choice.
+-   Can move functions between `api` and `impl` without visibility changing.
+
+Disadvantages:
+
+-   The `api` file's primary purpose is to expose APIs, and so it may be more
+    natural for developers to assume things there are public.
+-   Inconsistent with "default public" behavior on classes.
+
+We are switching to default public in `api` files for consistency with class
+behaviors.
+
+### Default `impl` to public
+
+Noting that we default `api` to public, we could similarly default `impl` to
+public.
+
+Advantages:
+
+-   Can move functions between `api` and `impl` without visibility changing.
+
+Disadvantages:
+
+-   Everything in an `impl` file must be private unless it's a separate
+    definition of an `api` declaration. As a consequence, everything declared in
+    the `impl` file would need to be explicitly `private`.
+
+In order to avoid the toil of explicitly declaring everything in the `impl` as
+`private`, `impl` will be `private` by default. As a consequence of being the
+default behavior, no `private` should be specified, just as `public` is not
+allowed in `api` files.
+
+Note the visibility behavior can be described as making declarations the most
+visible possible for its context, which in `api` files is `public`, and in
+`impl` is `private`.
+
+### Make keywords either optional or required in separate definitions
+
+When a prior declaration exists, keywords are _disallowed_ in separate
+definitions. We could instead allow keywords, making them either optional or
+required. This would allow developers to determine visibility when reading a
+definition.
+
+The downside of this is that optional keywords can be confusing. For example:
+
+-   `api` file:
+
+    ```
+    class Foo {
+      private fn Bar();
+      private fn Wiz();
+    };
+    ```
+
+-   `impl` file:
+
+    ```
+    fn Foo.Bar() { ...impl... }
+    private fn Foo.Wiz() { ...impl... }
+    fn Baz() { ...impl... }
+    ```
+
+In an "optional" setup, the above is valid code. However, the lack of a
+`private` keyword on `Foo.Bar` may lead a developer to conclude that it's public
+without checking the `api` file (particularly because `Foo.Wiz` is explicitly
+private), when it's actually private. This is an accident that could also occur
+on refactoring; for example, removing the keyword on the `impl` version of
+`Foo.Wiz` would be valid but does not make it public.
+
+A response may be to make keywords required to match, so that reading the `impl`
+file would have a compiled guarantee of correctness, avoiding confusion.
+However, consider a similar example:
+
+-   `api` file:
+
+    ```
+    class Foo {
+      fn Bar();
+      private fn Wiz();
+    };
+    ```
+
+-   `impl` file:
+
+    ```
+    fn Foo.Bar() { ...impl... }
+    private fn Foo.Wiz() { ...impl... }
+    fn Baz() { ...impl... }
+    ```
+
+In this example, `Foo.Bar` is public, and that may lead developers to conclude
+that `Baz` is public. This could be corrected by requiring `private` on `Baz`,
+but we are hesitant to do that per
+[Default `impl` to public](#default-impl-to-public).
+
+There is still some risk of confusion if the forward declaration and separate
+definition are both in the `api` file. For example:
+
+```
+private fn PrintLeaves(Node node);
+
+fn PrintNode(Node node) {
+  Print(node.value);
+  PrintLeaves(node);
+);
+
+fn PrintLeaves(Node node) {
+  for (Node leaf : node.leaves) {
+    PrintNode(leaf);
+  }
+}
+```
+
+In this, a reader may read the `PrintLeaves` definition and incorrectly conclude
+that it is implicitly `public` because (a) it has no keywords and (b) it is in
+the `api` file. This will be addressed as part of
+[Open question: Calling functions defined later in the same file #472](https://github.com/carbon-language/carbon-lang/issues/472#issuecomment-915407683).
+
+Overall, the decision to _disallow_ keywords on separate definitions means that
+`impl` files shouldn't have any visibility keywords at the file scope (they will
+on classes), which is considered a writability improvement while keeping the
+`api` as the single source of truth for `public` entities, addressing
+readability.