Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add Support for Indexers and Ranges #605
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
base: draft-v8
Are you sure you want to change the base?
Add Support for Indexers and Ranges #605
Changes from all commits
369ca4f
d77d147
1f97b8e
c3047be
3813628
54c4a55
56e0322
1ef3ab2
c051a5d
8ba0cfa
18319e5
7cb2cd5
3233609
d699f4f
0b81b47
a007699
8895e78
d8b297d
53d51fd
12b4632
f62b30f
cf03cbd
9e74541
5157c57
39a0fdd
7c67270
fb5703c
3981944
7d53e7b
5b596d0
d1a8fec
faf2eac
0b88e74
b2873d8
cca7304
cd2a008
1877160
be2d311
dd7764e
f651978
16818f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm…
A single plank is countable (in the sense intended)?
Maybe:
with a x-ref to wherever collection gets defined (see #1250).
Note: Qualifying, “collection type”, also avoids integers not be countable – which could otherwise confuse the numerate 😉
Note: This doesn’t address “In case both
Length
andCount
are present,Length
will be preferred.” found here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While having general terms is good this particular term currently only exists to be referenced in the new clauses “Implicit Index support” and ”Range Index support”. It would be better to move this to down to those two clauses, possibly even removing it as defined term altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this to the general clause of indexable sequences, where we're introducing other terms that are only relevant to that. (This doesn't address the plank example, but at least suggests where countable types are relevant.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clause & its subclauses might be better after §15.9 Indexers, or as a subclause of §15.9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Punt on this until the rest is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref: #1250 a Proposal to Define Some Container-Related Terms
I think this definition of indexable sequence (or collection) is not right.
#1250 has a renaming and different definition:
#1250 also defines collection:
I”m not sure where the “polymorphic” in the collection definition comes from, but it does correctly drop the “same type” of indexable sequence.
The term “ordered set” allows the reading that it is the elements that are ordered (as in [partially] ordered sets and
SortedSet<T>
), which they need not be. It is the indicies that are ordered… usually.System.Index
indicies by themselves are actually not ordered and the type does not implementIComparable<Index>
justIEquatable<Index>
. They are ordered only w.r.t a particular collection, which has someLength
, this is because indicies can be end-relative.Maybe:
Or something like that…
This is similar to the definition in #1250, but without the underlying definition of collection (for now…).
The definition of range here doesn’t address that a C#
a..b
Range
does not includeb
, or that a C#Range
may not be a contiguous set of ordered indices (consider1..^Int.MaxValue
which is a validRange
to construct but useless to use!).So there is a disparity between the term “range” and C#’s type
Range
– that might be confusing…There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest either "sequence" or "collection" instead of "set" as well. Likewise "subsequence" instead of "subset"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Replacement comment, I hit add too quickly]
@jskeet dropping set is a good idea.
But are we taking about sequences, which by (English language) definition are ordered, or collections which are not?
Do we need to make it clear that any ordering here comes from the index type being ordered, not the elements?
Indices do not need to be ordered per se, but a range of indices implies ordered indices, and indexable sequence/collection is being defined to be used by ranges.
Which gives us:
Too detailed/convoluted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question under discussion: does
Dictionary<TKey, TValue>
count as an indexable sequence? (Or rather, do we want it to?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting types:
int
System.Index
long
(just for arrays)We may want to limit this to integer-based indexes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a
Dictionary<int, string>
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also had some discussion about what "ordered index" means (especially when
System.Index
isn't inherently ordered).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Should this be a countable type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a slice have to be of a countable type? You can write an uncountable type with an indexer accepting a range, and make it fail at execution time if either end of the range is "from the end".
I agree it will usually be of a countable type, just trying to explore...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jskeet – In C# terms a slice made from a
Range
has to be of a countable type asRange
is built uponIndex
, andIndex
requires a length for it to be converted into an integer offset…However a non-countable type could provide the same underlyingSlice
method…Looking at your query made me think how careful we have to be with terms here.
No, an indexer takes an index as its argument, which does not need to be of type
Index
, or evenint
. So “…index is represented by…” is wrong, we’re using “index” to both mean a value of any type used as an index and to mean the (int
,bool
) pair embodied bySystem.Index
…I think we might yet need a lot of careful wordsmithing. Drat!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could specify here the range of valid values that may be provided with the
^
operator, similar to the "0 through N-1" text earlier on the line. In this case it would be "1 through N."Also, nit:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnm2 – 0 through N for
^
to allow for^0
? Otherwise 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definition of indexable sequence, both original and revised versions, do not fix the type of the index. Here the type is fixed to being an integer number (it does not state the actual type).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could mention here that either or both ends may be omitted, each referring then to its appropriate extremity within the available range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine as is. The other forms of
..
are in the linked section.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, a slice might be obtained using a range – or it might fail, a range can be constructed using the range operator…
TBD: Wordsmith a suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TBD from the preceding comment:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it typical in the spec to mention jagged arrays as though they were separate from single-dimensional arrays? I had thought of jagged arrays as a special case of single-dimensional arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might consider it a note:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @jnm2 here – C# doesn’t have jagged arrays per se, it supports single-dimensional arrays where the element type can be an array which:
I would just drop the reference (and the
!
), x-ref fixed as per @BillWagner comment below:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"All single-dimensional are indexable sequences" - this needs to be "All single-dimensional arrays are indexable sequences"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array access is now 12.8.12.2:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also say that these arrays are countable as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jskeet – I’ve now added that to the suggested definition of slice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And countable? (I won't keep adding this...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jskeet – Once we’ve the definition of indexable narrowed down to what is needed for this feature then countability (if it still exists as a concept) will be implied as using an
Index
relies on it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One sentence felt hard for me to follow. Does this version achieve the same goals you're looking to achieve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like Joseph's wording here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For convenience updating @jnm2’s suggestion with @BillWagner x-ref update:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indexer access moved:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both this section and the corresponding ranges section, I propose we rework them to introduce a pattern based description, following the format used for
async
return types, LINQ, and so on.The concept is that a countable sequence has a certain pattern for indexer, Count, Length, etc.
A type can provide support for ranges and index operations by implementing a pattern: Indexer that takes
System.Index
orin System.Index
etc.Slice
method that takes aSystem.Range
. Then describe ref variants.Finally, for arrays and other types the compiler recognizes as countable, the compiler can synthesize the lowering of the indexer operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be off on a tangent here, and I'm happy to get a better understanding of what the aims are in this sentence, but here goes 😁
If optional arguments are mentioned, is
params
implicitly considered an optional argument, and if not, should it be mentioned alongside optional arguments here?Also, the Roslyn compiler is happy with this code, which does not have an argument of type
System.Index
:In addition, due to implicit conversions, the type need not be
System.Index
so long as overload resolution succeeds:The compiler is also happy with the indexer having a
dynamic
parameter, etc.We also didn't mention refness of the parameter (
in
is allowed, butref
andout
are not), etc.I've observed the compiler team defining this general concept in terms of being able to bind arguments to an applicable overload. If we defined things the same way in the spec somewhere, we could refer to that definition. Thereby maybe we could sidestep all the gotchas around repeating ourselves exhaustively, as to what allows binding to succeed for these features.
There are other instances below to which this would also apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the change be this simple:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tangents are welcome! A core part of the task is looking into all the nooks’n’crannies that might not be covered in the source material.
How an indexer access is resolved to a particular indexer is already covered in §12.8.11.3 Indexer access which in turn relies on §12.6.4 Overload resolution – between them these cover such things as optional parameters,
params
and implicit conversions. So this para is, incompletely as you have identified, echoing part of the text it is actually referencing.So rather specifying indexers that have an argument of type
Index
, we need to specify in terms of indexers that accept/can be applied to a single argument of typeIndex
and then refer to §12.8.11.3 which covers all the cases.Suggested rewording:
Once the para is reduced to this it is clear it is stating the blindly obvious – before
System.Index
you could write an indexer taking any type of argument you wish, after you still can. Do we really need this at all?Following below there may be further suggestions changing “take” to ”accept”/”apply” – this and those need to be accepted/rejected as a whole.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from anything else: I think these should refer to parameters, not arguments. There's no such thing as an optional argument, only an optional parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jskeet – I think the use of “argument” is correct in my suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the preceding para has, hopefully, just stated the obvious – that adding another indexer is nothing new, I question whether this really needs an example which visually separates the previous para from the next clause that builds upon it.
However I accept this might not be a universal view! 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nothing else, we can reduce the clutter using expression bodies:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jskeet – that’s an improvement 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be clarified. The implementation only behaves this way in regard to the call site which calls this synthesized indexer. It does not, for instance, behave as if it provides this indexer within the class definition itself when the class is being compiled:
Same again below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnm2 – Hmm, I read “shall behave as if it provides” (emphasis added) as clearly stating what you’d like clarified – but folk do read things differently. Can you suggest some wording?
BTW I would expect that an implementation could choose to add an actual indexer to the type should it wish and be compliant, so if folk agree and don’t think this wording allows that then some wordsmithing will be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally not expect it to add an actual indexer to the type, as that could then be detected as a breaking change later if someone adds an actual indexer with a different parameter name.
But I do think it could potentially be clarified in terms of what's doing the "providing", e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In contrast to another comment thread, this is an example where "able to bind
(int)
" is not the only requirement, since subsequent optional parameters do throw it off. 👍Is parameter refness important to mention?
in int
is not supported, for instance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Replaced] (My fears were justified)
For the refness question, maybe:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I think we should talk about the declared parameters, rather than "taking an argument".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think by this list, the following should not error, but it does:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'range' version of this called out that the return type was the same, including 'ref'. That is true here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand what's meant by "the same get and set members" here in the first place... My guess is that this is to do with "if the int-based indexer is readable, then so is the provided Index-based indexer; likewise is the int-based indexer is writable, then so is the provided Index-based indexer" but I could be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting descriptive to specification language:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further to my last comment, there is no context here as to why either
Length
orCount
should be used at all – there is no mention ofGetOffset
is there is in expressions.md line 2242. And as commented on the latter, surely we don't need this stuff in two places?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
section change:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Simple" seems to be a subjective term. What would the goal be to communicate here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnm2 – maybe “straightforward” which doesn’t carry the same risk of offense as “simple”, or may be:
Wordsmith away folks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed as a note, and arrays are left out of the list on the previous line. It seems like arrays should be given equal special-case status as strings. These are the two types that are the exception to needing a method named Slice.
If the Roslyn compiler can't find the method RuntimeHelpers.GetSubArray, it still behaves as if it provides the range indexer, and then fails at a later stage with "CS0656 Missing compiler required member 'System.Runtime.CompilerServices.RuntimeHelpers.GetSubArray'." So the presence of this method is not a determining factor for whether the Roslyn implementation behaves as if it provides the Range indexer.
I didn't check what happens if
System.String
doesn't have aSubstring
method because that would take longer to test, but perhaps the logic is this:Then, mentions of the Substring and GetSubArray methods could be notes informing readers what will happen in those two special cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a different method is being used for arrays then it needs to be specified in normative text, not as an informative note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we need to think about whether
GetSubArray
is required, or whether it would be okay for an implementation to actually provide a method calledSlice
instead...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
section change:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should
ref readonly
be mentioned too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ref_kind, I suspect it just predates this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence doesn't hold for arrays.