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 Support for Indexers and Ranges #605

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

Conversation

RexJaeschke
Copy link
Contributor

@RexJaeschke RexJaeschke commented Aug 7, 2022

Before you look at the detailed edits, it likely will be useful to read the following:

  • 14.7 Properties|14.7.1 General, which defines countable as it pertains to a type.
  • 14.(x) Indexable sequences, which defines indexable sequence.

@RexJaeschke RexJaeschke marked this pull request as draft August 7, 2022 14:15
@RexJaeschke RexJaeschke added this to the C# 8.0 milestone Aug 7, 2022
standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Show resolved Hide resolved
standard/classes.md Show resolved Hide resolved
standard/classes.md Show resolved Hide resolved
standard/classes.md Show resolved Hide resolved
@RexJaeschke RexJaeschke changed the base branch from draft-v7 to draft-v8 August 8, 2022 13:05
@RexJaeschke RexJaeschke changed the title Indexers and ranges Add Support for Indexers and Ranges Aug 8, 2022
@RexJaeschke
Copy link
Contributor Author

RexJaeschke commented Aug 11, 2022

[I raised the questions below on https://github.com/dotnet/docs/issues/30512, and got replies, which I need to factor in to this Draft PR.]

I'm transforming this proposal into a Draft PR for the C# Standard, and have several questions:

Q1. Optional Members in Index and Range: The proposal suggests that some members in these types are optional, such as Range's StartAt, EndAt, and All. Which members are required for these types? Why not require all those provided by MS's implementation?

Specifically, the MS proposal states:

The .. syntax for System.Range will require the System.Range type, as well as one or more of the following members:

namespace System
{
    public readonly struct Range
    {
        public Range(System.Index start, System.Index end);
        public static Range StartAt(System.Index start);
        public static Range EndAt(System.Index end);
        public static Range All { get; }
    }
}

The .. syntax allows for either, both, or none of its arguments to be absent. Regardless of the number of arguments, the Range constructor is always sufficient for using the Range syntax. However, if any of the other members are present and one or more of the .. arguments are missing, the appropriate member may be substituted.

MS's feedback on my Q was:

The only one required is the constructor: we will fall back to it if any of the others are missing. As to why it was specified this way, I'm not certain, and unfortunately Andy (who implemented it) just went on leave for a few months.

As a result, I'm omitting any mention of the optional members StartAt, EndAt, and All, and defining things in terms of the constructor.

Q2. Range indexer setter: What if anything should we say about the implicit and explicit setter for a Range indexer? Certainly, one can define a setter for a user-defined type; however, it is not obvious as to what such a setter would do, especially since it must be used on the left-hand side of assignment taking a right-hand side of the same type as the index returns. In the case of type BitArray that would mean something like ba1[range1] = ba2, or perhaps ba1[range1] = ba2[range2]. As far as I can determine, the operations one might like to implement using such a setter are probably best implemented via a named method. In any event, for a compiler-generated Range indexer, attempting to use its setter results in the error message “CS0131 The left-hand side of an assignment must be a variable, property or indexer,” which suggests the generated indexer has no setter. If that is the case, we should say that, perhaps by stressing that the result of a Range indexer is not a variable, so as such, it can't be used on the lhs of an assignment.

MS's feedback on my Q was:

I think we should say nothing about it, or at most say that no setter is synthesized if one does not exist. I think the rest of the rules should just fall out from this, as the error implies.

I chose to say nothing about the setter, which means that attempting to use such a setter results in unspecified behavior.

@RexJaeschke
Copy link
Contributor Author

When adapting the MS proposal, I invented the term indexable sequence. The rationale for that name choice follows.

Various MS-hosted on-line pages use the term sequence. This word is already used quite a bit in the C# spec, in both a general sense as well as being defined in the context of query expressions. §11.17 Query expressions|§11.17.1 General states:

A query expression begins with a from clause and ends with either a select or group clause. The initial from clause may be followed by zero or more from, let, where, join or orderby clauses. Each from clause is a generator introducing a range variable that ranges over the elements of a sequence. Each let clause introduces a range variable representing a value computed by means of previous range variables. ….

That definition is not applicable to indexes and ranges! The MS-provided proposal uses collection; however, that implies enumerable support, which is not required by indexes and ranges. (BTW, although it is used a lot in the C# spec, the term collection is not defined!) As such, rather than overload an existing term or invent a completely different one, I came up with indexable sequence.

@RexJaeschke
Copy link
Contributor Author

The MS proposal, section “Implicit Range support,” provided a very detailed discussion of how to transform a pair of Indexes into a call to Slice depending on the form of the range used. I did not retain this in the final proposal, as I saw no point in doing so. Given a start and end index, it is a simple matter to compute the length in all cases regardless of range format!, as I show in my range indexer implementation in “Explicit range support.”

@BillWagner BillWagner force-pushed the indexers-and-ranges branch 2 times, most recently from c810a4a to 867ba90 Compare February 6, 2023 13:27
@RexJaeschke RexJaeschke added the type: feature This issue describes a new feature label Jul 22, 2023
@BillWagner
Copy link
Member

rebased on the latest draft-v8 branch on 09/26/2023

@BillWagner BillWagner closed this Sep 26, 2023
@BillWagner BillWagner reopened this Sep 26, 2023
@RexJaeschke RexJaeschke added the Review: pending Proposal is available for review label Oct 13, 2023
@RexJaeschke RexJaeschke marked this pull request as ready for review January 11, 2024 14:10
@RexJaeschke RexJaeschke reopened this Aug 25, 2024
@jskeet jskeet self-assigned this Sep 4, 2024
- added non-virtual requirement to 2 x imp-generated indexers.
- described `GetOffset` semantics.
- removed erroneous text w.r.t optional trailing params on `this[int]`.
@RexJaeschke
Copy link
Contributor Author

@KalleOlaviNiemitalo I just revisited this PR and resolved all of Kalle's comments. While doing that, I found a requirement I'd added that was incorrect, so I removed that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: pending Proposal is available for review type: feature This issue describes a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants