Skip to content

Add support for Index & Range #1369

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

Open
wants to merge 4 commits into
base: draft-v8
Choose a base branch
from

Conversation

Nigel-Ecma
Copy link
Contributor

Alternative to #605

  • Add support for Index & Ranges
  • Details of types and "implicit support" in new chapter, temporarily at end to reduce lots of changes due to renumbering
  • Attempts to avoid all implementation specific details from source material
  • Covers dynamic binding, associativity, etc.
  • Updates grammar test reference files (adding a new layer to the expression stack changes the tree but not the parse). These files can be ignored for review (they are machine generated).
  • Adds a new grammar test with Index & Range samples

@Nigel-Ecma Nigel-Ecma requested a review from a team July 12, 2025 04:06
@RexJaeschke
Copy link
Contributor

RexJaeschke commented Jul 12, 2025

Editorial Nits

I can take care of the following once we merge this PR:

  1. Change all code block starts from CSharp to csharp.
  2. Change all "indicies" to "indices".
  3. Change all "[C|c]hapter" to "[C|c]lause" Done by PR Replace chapter/section with clause/subclause, and clause with subclause, as appropriate #1373.
  4. Make consistent with the existing spec the use of "that" vs. "which" and associated punctuation.
  5. Add test harnesses to all examples.
  6. Change all "must" in normative text to "shall".
  7. Make sure that all the (currently) hard-coded references in the new clause 24, "Extended Indexing and Slicing," are made relative.

@RexJaeschke
Copy link
Contributor

In email Nigel sent to active TG2 members on 2025-07-12, Subject: [TG2] Re: PR#1369 Add support for Index & Range, he wrote

The types added in §C.3 have documentation comments. Good, bad or ugly?

I've generalized that question by linking (the currently Parked) Issue 1135 to here, and asked for priority on that Issue next call.

@Nigel-Ecma
Copy link
Contributor Author

Thanks for quick the review, I'll work my way down.

Editorial Nits

I can take care of the following once we merge this PR:

  1. Change all code block starts from CSharp to csharp.

Will be in next commit.

  1. Change all "indicies" to "indices".

This one beat my spell-checker, will be in the commit.

  1. Change all "[C|c]hapter" to "[C|c]lause"

Not sure about this one.

While “chapter” does not occur in the text often (I think it is used a few times in 3 out of 7 versions) when it does it is referring to a whole §N (no .M), i.e. a whole chapter ;-) Further it is used in the page headings in most versions including the last one, v7. I like that it unambiguously refers to the whole chapter. We typically use “subclause” (which GitHub has now replaced twice with “subclass” and I've fixed it, I may not spot this every time!) for a §N(.M)+, but not always, sometimes I think to refer to a chapter, and sometimes to refer to a language construct – some examples:

  • subclause, §15.7.3 “This clause applies to both properties…”
  • chapter, §12.1 “This clause defines the syntax…”
  • language construct, §21.4 “Once a matching catch clause is found…”
  • most things, §6.2.4 “The syntactic grammar of C# is presented in the clauses, subclauses, and annexes that follow this subclause.”
  • does clause/subclause include nested subclauses, sometimes we elaborate:
    • §23.1 “The remainder of this clause, including all of its subclauses, is conditionally normative.”
    • §12.8.9.1 “The remainder of this subclause and the following sibling subclauses are conditionally normative.”

I can see an argument for “chapter”, a review of how (sub)clause is used (yikes!), and acknowledge the usages I added are currently the only ones in v8.

  1. Make consistent with the existing spec the use of "that" vs. "which" and associated punctuation.
  2. Add test harnesses to all examples.

These two are definitely more @RexJaeschke’s area than mine, I happily leave them to him, thanks :-)

@RexJaeschke
Copy link
Contributor

Re chapter/clause usage. Until we adopted the wrapper spec for ISO to avoid complying completely with their style, we followed ISO rules in this regard. And we might as well keep doing that to be consistent.

  • An ISO spec had clauses rather than chapters.
  • An ISO spec had subclauses rather than sections.
  • An ISO spec does not have subsubclauses; subclause is used for all subordinate levels.

@Nigel-Ecma Nigel-Ecma self-assigned this Jul 13, 2025
@Nigel-Ecma
Copy link
Contributor Author

Re chapter/clause usage. Until we adopted the wrapper spec for ISO to avoid complying completely with their style, we followed ISO rules in this regard. And we might as well keep doing that to be consistent.

  • An ISO spec had clauses rather than chapters.
  • An ISO spec had subclauses rather than sections.
  • An ISO spec does not have subsubclauses; subclause is used for all subordinate levels.

Isn't it also a requirement that all text is in subclauses (the reason for all the §N.1 General subclauses)?

So a clause has no text in it and something like “see clause §N“ can be taken to mean the whole clause and all the subclauses, i.e. what “chapter” has been used for occasionally over the versions and now in this PR.

Does this mean something like “see clause §N.M” should use subclause, or does the “.M” allow clause to be used?

Anyway, if this is definitive we need an issue to remove “Chapter” from the page headers in the final Standard (where it is in most ECMA releases).

I suspect that clause might be misused to mean subclause, and that it is not maybe clear that clause means the whole chapter, so so cleanup might be in order. There are 107 uses (in draft-v8 at the point this PR branched), I think most refer to language clauses so are fine so the job isn’t too daunting…

@RexJaeschke
Copy link
Contributor

@Nigel-Ecma:

Isn't it also a requirement that all text is in subclauses (the reason for all the §N.1 General subclauses)?

Yes, Any clause or subclause containing a subclause cannot have its own text. Specifically, "hanging paragraphs are not allowed."

So a clause has no text in it and something like “see clause §N“ can be taken to mean the whole clause and all the subclauses, i.e. what “chapter” has been used for occasionally over the versions and now in this PR.

Exactly! This is the reason ISO has this requirement. A reference of §N always means all of N and its subordinates, and a reference of §N.M always means all of N.M and its subordinates.

Does this mean something like “see clause §N.M” should use subclause, or does the “.M” allow clause to be used?

N.M is a subclause, not a clause. ISO only uses the word "Clause" in a reference when the whole clause is intended. They do not use "subclause" for N.M, but rather, say nothing, so "see §N.M" is used instead. That said, ISO does not allow the use § in references, but I've always has it in documentation I've written, and I introduced it in V1.

Anyway, if this is definitive we need an issue to remove “Chapter” from the page headers in the final Standard (where it is in most ECMA releases).

Good observation. When we are ready to push out a new edition, Jon gives me the final generated Word version and I do light formatting on it before submitting it to Ecma. I just added to my list of "things to do in that process" to change the clause headers from "Chapter" to "Clause." Or we could simply omit that word altogether. In any event, we don't say "Annex" for each Annex header.

I suspect that clause might be misused to mean subclause, and that it is not maybe clear that clause means the whole chapter, so so cleanup might be in order. There are 107 uses (in draft-v8 at the point this PR branched), I think most refer to language clauses so are fine so the job isn’t too daunting…

Once we've discussed this I can clean this up.

@RexJaeschke RexJaeschke added the meeting: priority Review before meeting. Merge, merge with issues, or reject at the next TC49-TC2 meeting label Jul 14, 2025
@Nigel-Ecma
Copy link
Contributor Author

@RexJaeschke wrote:

[snip] That said, ISO does not allow the use § in references, but I've always has it in documentation I've written, and I introduced it in V1.

Well of course they don’t, it is the section sign and they are clearly anti-section ;-), we like §, M§GA (ducking) ;-) Are there enough ;-)’s?

With the plan to remove all the occurrences of chapter and aim for consistency on the use of (sub)clause, the need for things like “this subclause and its subclauses”, et al., the four that are down to me will be gone in the next commit.

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Lots of tiny nits/typos, but I'm generally liking the approach here a lot.

One always expects a few typos to get through, a “must” or two instead of “shall”, etc., but the first commit was littered with the detritus of an incrementally developed document – a very poor show, no excuses just apologies.

This commit hopefully removes the distraction of the detritus making reviewing a lot easier! Thanks especially for the Herculean efforts of Rex & Jon, anything remaining is down to me of course.
@Nigel-Ecma
Copy link
Contributor Author

If a comment thread was left open and not marked as resolved before the second commit it is still valid – ignore GitHib’s “Outdated” tag.

The failure of the renumbering check is fixed, any reference to §24.1.1 is now correctly §24.1 in the master and there is no need to report the issue.

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

Good work overall. A few comments and suggestions.

Index operator ^(int x);
```

The hat operator is not overloadable (§12.4.3).
Copy link
Member

Choose a reason for hiding this comment

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

While I'd prefer a single word, I'd prefer something like "reverse index" or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is called both the hat operator and the index from end operator in the various source materials, with the LDM at least recording a preference for the former at some point. I thought the latter was too much of a mouthful. Let’s see what the consensus is, remembering to consider list of definitions in §24.1 (from-start, from-end & past-end index) – hat (and other names/terms) are not fixed.

Copy link
Contributor Author

@Nigel-Ecma Nigel-Ecma Aug 4, 2025

Choose a reason for hiding this comment

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

I will leave this one unresolved as I don’t recall we discussed it. The question is should the term be:

  • hat (from source material and plenty of hits using DDG so it might qualify as lingua franca if we care ;-))
  • from-end index (mainly for the index rather than the operator – from source material and also hits using DDG);
  • reverse index (@BillWagner above); or
  • something else?

@@ -0,0 +1,296 @@
# 24 Extended Indexing and Slicing

> **Review Note:** This new clause, currently (§24), is placed here temporarily to avoid text changes due to renumbering occurring in chapters & clauses otherwise unaffected by the PR. Its final placement is not yet determined, however between the Arrays ([§17](arrays.md#17-arrays)) and Interfaces ([§18](interfaces.md#18-interfaces)) chapters might be suitable – other placements can be suggested during review. It can be relocated later with just a simple edit to `clauses.json`.
Copy link
Member

Choose a reason for hiding this comment

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

After arrays.md might make the most sense. Sadly, it makes me wonder if interfaces should precede arrays, instead of the other way around.

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Some comments (don't merge just yet), but I'd really like to get the first pass at this merged either during or shortly after the meeting, and additional issues filed if necessary.


> *Note:* No checking is specified as the expected use of the result is to index into a sequence with `length` elements, and that indexing operation is expected to perform the appropriate checks. *end note*

`Index` implements `IEquatable<Index>` and values may be compared for equality. However `Index` values are not ordered and no other comparison operations are provided.
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 note that two Index values can resolve to the same concrete int index value when applied to the same countable collection, but still be unequal due to one being from-start and the other being from-end? Maybe it's obvious enough. (Or maybe we just state that equality is based on both the Value and IsFromEnd properties being equal.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jskeet – Hmm, its comparing abstract indices so it should be obvious… but if you’d like to craft a Note I see no harm. Or I could spell it out as you suggest. Another one for the meeting.


If either or both operands in a range expression have the compile-time type `dynamic`, then the expression is dynamically bound ([§12.3.3](expressions.md#1233-dynamic-binding)). The compile-time type of the result is always `Range`.

A lifted ([§12.4.8](expressions.md#1248-lifted-operators)) form of the range operator is also predefined.
Copy link
Contributor

Choose a reason for hiding this comment

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

Toying with lifted operators:

using System;
class C
{
    // means ^default(int), not ^default(int?)
    static Index? I => ^default;

    // means I..default(Index), not I..default(Index?)
    static Range? R => I..default;

    // error CS8310: Operator '*' cannot be applied to operand 'default'
    static int? K => ((int?)4) * default;
}

Does something in the specification explain why default is allowed as an operand in lifted .. but not in lifted *? Or is this a compiler bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KalleOlaviNiemitalo – Can you add an issue for this to be looked into? Or would you prefer we do it?

Copy link
Member

Choose a reason for hiding this comment

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

I believe the first two are target-typed, and the third is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given @gafter’s comment this might come down to ^ & .. not being overloadable, while * is which requires both operands have types to resolve overloading. If that is correct I wouldn’t say it exactly jumps off the page…

…left untouched); other reported nits; and the numbering oops (24.1.1 -> 24.1 as header was removed)

If either or both operands in a range expression have the compile-time type `dynamic`, then the expression is dynamically bound ([§12.3.3](expressions.md#1233-dynamic-binding)). The compile-time type of the result is always `Range`.

A lifted ([§12.4.8](expressions.md#1248-lifted-operators)) form of the range operator is also predefined.
Copy link
Member

Choose a reason for hiding this comment

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

I believe the first two are target-typed, and the third is not.

@jskeet
Copy link
Contributor

jskeet commented Jul 30, 2025

We will try to resolve remaining issues offline.

@BillWagner BillWagner added this to the C# 8.0 milestone Jul 31, 2025
- Description of the semantics of `Range` expanded in
  ranges.md and uses in expressions.md xref to there
  for the semantics
- All references to certain introduced features not
  being available in dynamic bound code elided.
- Various wording tweaks.
@Nigel-Ecma
Copy link
Contributor Author

Post-meeting changes made.
Most conversations marked as resolved, a few left open – authors please check and (un)resolve if neccesary.
Removing "Draft" status and re-requesting reviews.

@Nigel-Ecma Nigel-Ecma marked this pull request as ready for review August 8, 2025 03:22
Copy link
Contributor

@MadsTorgersen MadsTorgersen left a comment

Choose a reason for hiding this comment

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

This is a very impressive PR! I read through everything in the standard folder, and as far as I can see, the whole thing checks out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting: priority Review before meeting. Merge, merge with issues, or reject at the next TC49-TC2 meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants