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

Code and name organization #107

Merged
merged 101 commits into from
Oct 14, 2020
Merged

Code and name organization #107

merged 101 commits into from
Oct 14, 2020

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Jul 8, 2020

Files, packages, libraries, and imports for Carbon.

Contributors: chandlerc, fowles, tkoeppe, zygoloid

@jonmeow jonmeow added WIP proposal A proposal labels Jul 8, 2020
@googlebot googlebot added the cla: yes PR meets CLA requirements according to bot. label Jul 8, 2020
proposals/p0107.md Outdated Show resolved Hide resolved
proposals/p0107.md Outdated Show resolved Hide resolved
proposals/p0107.md Outdated Show resolved Hide resolved
proposals/p0107.md Outdated Show resolved Hide resolved
proposals/p0107.md Outdated Show resolved Hide resolved
proposals/p0107.md Outdated Show resolved Hide resolved
proposals/p0107.md Outdated Show resolved Hide resolved
proposals/p0107.md Outdated Show resolved Hide resolved
proposals/p0107.md Outdated Show resolved Hide resolved
proposals/p0107.md Outdated Show resolved Hide resolved
proposals/p0107.md Outdated Show resolved Hide resolved
@jonmeow
Copy link
Contributor Author

jonmeow commented Jul 20, 2020

Quick reminder all! This proposal is a WIP, and I have not posted it to ideas. I was simply copying a legacy doc for visibility. As noted in the PR description, I have not had time to edit it, and that may take a bit. I hope a rewrite will address your comments, @josh11b @tkoeppe !

- We should make it easy to refactor code, including moving code between
files. This includes refactoring both by humans and by developer
tooling.

Copy link
Contributor

@jsiek jsiek Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goals/philosophy should include something that might be called "the library user's bill of rights".
Some of the following is perhaps stated elsewhere, but here is a great place for it, and furthermore, there
is a long and rich history of programming language designs getting this wrong! (e.g. C++ and Haskell.)

The person importing a library should have complete control over which names come into scope
and the ability to rename the imported entities. The crucial scenario to think about is that the
person is importing from two libraries and those two libraries were developed by people that don't know or care about each other, or perhaps even dislike each other! So the two libraries may have lots of name conflicts. But nevertheless, it should be possible for someone to import both libraries and apply the appropriate renamings to resolve the conflicts they care about.

Copy link
Contributor Author

@jonmeow jonmeow Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name conflicts are noted in the actual proposal file (p0107.md) as out of scope for this proposal; either the aliasing or name lookup proposal should address it more directly. While it's an important issue, they are nuanced discussions that require a more complex discussion, this proposal is already quite large, and there's dissent around whether renaming imports would be necessary. I want to keep the proposal's scope at a level where it's straightforward to review, and so it is not a goal addressed by this feature design, at least at present.

I'm wary too of the meaning of "complete control". I think this overlaps with the idea of renaming, as responded to above.

Note, I think the goal on line 96 captures what you're trying to address, as far as it affects this design:

-   We should support libraries adding new structs, functions or other
    identifiers without those new identifiers being able to shadow or break
    existing users that already have identifiers with conflicting names.

Please make suggestions on phrasing if this isn't direct enough for you, although I'm trying to keep possible implementations of this goal to the rest of the design.

Please also note the "Broader imports, either all names or arbitrary code" alternative, which dives into the related tradeoffs a little more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a better place to address this concern, that's fine.

Regarding the goal on line 96, that doesn't quite cover this concern
because it only talks about the consequences to existing users of adding new identifiers.
But this concern also applies to new users.

Copy link
Contributor Author

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Responding to comments

- We should make it easy to refactor code, including moving code between
files. This includes refactoring both by humans and by developer
tooling.

Copy link
Contributor Author

@jonmeow jonmeow Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name conflicts are noted in the actual proposal file (p0107.md) as out of scope for this proposal; either the aliasing or name lookup proposal should address it more directly. While it's an important issue, they are nuanced discussions that require a more complex discussion, this proposal is already quite large, and there's dissent around whether renaming imports would be necessary. I want to keep the proposal's scope at a level where it's straightforward to review, and so it is not a goal addressed by this feature design, at least at present.

I'm wary too of the meaning of "complete control". I think this overlaps with the idea of renaming, as responded to above.

Note, I think the goal on line 96 captures what you're trying to address, as far as it affects this design:

-   We should support libraries adding new structs, functions or other
    identifiers without those new identifiers being able to shadow or break
    existing users that already have identifiers with conflicting names.

Please make suggestions on phrasing if this isn't direct enough for you, although I'm trying to keep possible implementations of this goal to the rest of the design.

Please also note the "Broader imports, either all names or arbitrary code" alternative, which dives into the related tradeoffs a little more.

@sidney13 sidney13 added decision: needs work and removed proposal rfc Proposal with request-for-comment sent out comment deadline labels Sep 29, 2020
is out of scope.

- Name lookup, including addressing conflicting names between imports and
names in the current file: Name lookup is likely to address this better,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
names in the current file: Name lookup is likely to address this better,
names in the current file: A separate name lookup proposal will address this directly,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer "is likely to" because "will ... directly" sounds like a really firm statement about the future, one which I don't think I'm going to be responsible for enforcing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most important word I wanted to include was "proposal" after "name lookup". Otherwise this sentence doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, clarified "name lookup design".

docs/design/code_and_name_organization.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +341 to +344
- API file paths will correspond to the library name.
- The precise form of this correspondence is undetermined, but should
be expected to be similar to a "Math/Algebra" library being in a
"Math/Algebra.carbon" file path.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had thought we would want to use the exact API file's path? That is, include the .carbon? But maybe not.

(I don't feel strongly about this either way, and don't think this is super important, mostly want us to be intentional in the choice)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should omit it -- however, that can always change in another, more targeted proposal. I do admit an assumption that we wouldn't want to repeat file extensions, if possible. This is particularly notable if we expect all impl files to be prefixed with the library name, e.g. "Math/Algebra.impl.carbon", "Math/Algebra.impl.MyDetail.carbon", at which point adding the ".carbon" to the library will end up obfuscating the path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it should be omitted, as it is just boilerplate noise. Including it will make it harder to support filesystems that use . as a directory separator.

- The precise form of this correspondence is undetermined, but should
be expected to be similar to a "Math/Algebra" library being in a
"Math/Algebra.carbon" file path.
- The package will not be used when considering the file path.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention that the API file path above is relative to some package-specific root? or not at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue for leaving that to be resolved later. I think "package-specific" could imply that each package has its own root, and I believe the discussion so far had been:

  1. A package may have multiple roots, such as one for human-authored files and one for auto-generated files. This could also be multiplied by separating public and package-internal APIs.

  2. A given root could arguably contain multiple packages, as was suggested for the Google use-case.

Thus the idea of making a quick remark about a package-specific root seems pretty limiting, and one that should be delayed for further proposals. I can particularly see how #2 may get arguments.

@jonmeow jonmeow added proposal accepted Decision made, proposal accepted and removed needs decision labels Oct 7, 2020
@sidney13
Copy link
Contributor

Approved on 2020-10-06. Approval announcement

@sidney13 sidney13 self-requested a review October 14, 2020 15:36
@jonmeow jonmeow dismissed chandlerc’s stale review October 14, 2020 17:51

Per discussion, chandlerc said he doesn't see a need to review again

@jonmeow jonmeow merged commit b17d7af into carbon-language:trunk Oct 14, 2020
@jonmeow jonmeow deleted the proposal-code-and-name-organi branch October 14, 2020 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants