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

Add normative-conventions.md; document agreed-upon coercion rules #136

Merged
merged 13 commits into from
May 26, 2024

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Sep 13, 2023

We got explicit consensus for these rules in the July 2023, September 2023, November 2023, and April 2024 meetings (notes not yet available for the April 2024).

#119 would introduce a spec-conventions.md. I think we shouldn't mix normative with editorial conventions, so I've made a different document. But the sole normative convention currently in that PR would be good to include in this new document.

normative-conventions.md Outdated Show resolved Hide resolved
normative-conventions.md Outdated Show resolved Hide resolved
normative-conventions.md Outdated Show resolved Hide resolved
@sffc
Copy link

sffc commented Sep 29, 2023

This PR doesn't yet include object-to-primitive coercion, but I said in plenary this week that I would post a writeup to document the impact this has on library authors. I'll post it here as a reply to this thread.

Note: As many of us do, I wear multiple hats. In this post, I wear my hat only as an i18n library author.

Newtypes / Wrapper Objects

A use case for coercion is the newtype idiom. The idiom is useful for attaching semantic meaning to low-level values. Coercion can be used to unwrap the newtype to its underlying primitive.

class WholeNumber {
  #value;
  constructor(value) {
    if (value < 0 || value % 1 !== 0) {
      throw new Error("Not a whole number: " + value);
    }
    this.#value = value;
  }

  [Symbol.toPrimitive](hint) {
    if (hint === "number" || hint === "default") {
      return this.#value;
    }
    if (hint === "string") {
      return String(this.#value);
    }
  }
}

let five = new WholeNumber(5);
console.log(`${five}`); // "5"

let nf = new Intl.NumberFormat("en", { maximumFractionDigits: five });
console.log(nf.resolvedOptions().maximumFractionDigits); // 5

Playground

Enums

Many libraries, including Intl, make use of string enums. Symbol.toPrimitive can be used to create an enum-as-a-class that converts itself when passed into an API that needs the string enum.

class RoundingMode {
  #id;
  constructor() {
    this.#id = "halfExpand";
  }
  static get ceil() {
    return this.construct("ceil");
  }
  static get floor() {
    return this.construct("floor");
  }
  static get halfExpand() {
    return this.construct("halfExpand");
  }
  private static construct(id) {
    let val = new RoundingMode();
    val.#id = id;
    return val;
  }
  [Symbol.toPrimitive](hint) {
    if (hint === "string" || hint === "default") {
      return this.#id;
    }
  }
}

let ceil = RoundingMode.ceil;
console.log(`${ceil}`); // "ceil"

let nf = new Intl.NumberFormat("en", { roundingMode: ceil });
console.log(nf.resolvedOptions().roundingMode); // "ceil"

Playground

Without Coercion

If manual coercion is required, the call sites become the following, which I find as a library author to be less readable and more error-prone. I would rather write newtypes and enums that "just work".

// Verbose Form:
let nf = new Intl.NumberFormat("en", { maximumFractionDigits: Number(five) });
let nf = new Intl.NumberFormat("en", { roundingMode: String(ceil) });

// Compact form to reduce code size:
let nf = new Intl.NumberFormat("en", { maximumFractionDigits: five|0 });
let nf = new Intl.NumberFormat("en", { roundingMode: ceil+"" });

@bakkot
Copy link
Contributor Author

bakkot commented Sep 29, 2023

If manual coercion is required, the call sites become the following, which I find as a library author to be less readable and more error-prone.

Hm. Surely the relevant perspective on callsites is that of library users, not of authors, right? And as a library user (and reader of such code), I find the explicit coercions easier to understand than the implicit ones. The implicit coercions look like bugs: maximumFractionDigits is a standard API which takes a number, not an object. You have to be pretty familiar with details of the language and of the library to understand why passing an object is not a bug in this case.

(Sidebar: the idiom for coercion to number I see most often is is +x, not x|0.)

@tabatkins
Copy link
Contributor

I strongly agree with @bakkot as a code author; I enjoy a good type pun as much as the next person, but passing a string to an API that expects a string is just straightforward and readable. Many (tho not all) callsites in Python that expect a number or a string will throw if given the wrong thing, and I don't find int(myVar) or str(myVar) calls to be at all onerous in those cases. (And, since I use mypy on all my code, I have to do it anyway to make the typechecker happy.)

I find relying on coercion to almost always sit on the "slightly too clever" side of things. Being slightly more explicit makes for better, more readable code imo, and the explicit conversion idioms (+foo and ""+foo) are short, idiomatic, and immediately recognizable for what they're doing.

@justingrant
Copy link

explicit conversion idioms (+foo and ""+foo)

FYI, these idioms are challenging for Temporal objects.

""+foo, will throw if foo is a Temporal object because valueOf throws. Is there another string-coercion pattern that we can recommend that won't call valueOf? Is ${foo} better? Or String(foo)?

+foo will also throw, but at least in that case other objects return NaN. So I'm less concerned about that one because there's no other idiom like Number(foo) that behaves differently than +foo for Temporal objects.

FWIW, I spent many weeks arguing collaborating with the React team about removing all ""+foo from React's codebase and replace it with String(foo). In the end, we agreed on a partial solution that left ""+foo in perf-critical paths but switched to String(foo) everywhere else. Context: facebook/react#22064

TypeScript is hopefully (microsoft/TypeScript#52773) going to make these problems easier to find by making it a compile error to use a built-in operator that calls valueOf with an operand whose valueOf returns void or never.

@tabatkins
Copy link
Contributor

""+foo will throw if foo is a Temporal object because valueOf throws

Interesting. That's definitely a challenge, then. Tho, in my limited experience with Temporal objects, you don't really want to toString() them anyway, so probably not a huge deal. ^_^ But yeah, the String(foo) or `${foo}` idioms are also fine, just a bit longer. Neither are problematic in practice imo.

normative-conventions.md Outdated Show resolved Hide resolved
@syg
Copy link
Contributor

syg commented Nov 20, 2023

I think the harms done by implicit coercion as the way we've designed standard APIs is pretty cut and dry: security bugs that pay out bounties, cost engineer time to fix, 0-day exploits, etc.

The claimed upside is flexibility and ergonomics afforded to users of libraries, and library authors wanting to confer that flexibility.

Both are good outcomes, and all other things being equal, we should trade off the upside of fewer security bugs and the downside of less flexibility towards the pole that is more impactful. My intuition is squarely that "fewer security bugs" is far more impactful as an outcome. My intuition is also that libraries or library users don't tend to want that flexibility, but I'd certainly be interested in seeing counterexamples. What I see seems to be that the ecosystem doesn't regularly use this flexibility, and that this flexibility is often deemed a smell or overly clever.

@justingrant
Copy link

Glad we're making these changes to enforce type safety. Thanks for pushing this.

normative-conventions.md Outdated Show resolved Hide resolved
Co-authored-by: Michael Ficarra <[email protected]>
Co-authored-by: Michael Ficarra <[email protected]>
@ctcpip ctcpip mentioned this pull request May 22, 2024
@bakkot bakkot merged commit 92a9f39 into main May 26, 2024
2 checks passed
@bakkot bakkot deleted the add-stop-coercing-things-pt-1 branch May 26, 2024 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants