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

Remove indexer from ref source of WebHeaderCollection #37949

Merged

Conversation

aik-jahoda
Copy link
Contributor

@aik-jahoda aik-jahoda commented Jun 16, 2020

The indexer is not implemented on WebHeaderCollection.

This fix extra indexer in the documentation: #4268. More details in the following comment: dotnet/dotnet-api-docs#4268 (comment)

@ghost
Copy link

ghost commented Jun 16, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@aik-jahoda aik-jahoda requested a review from a team June 16, 2020 07:54
@davidsh
Copy link
Contributor

davidsh commented Jun 16, 2020

The indexer is not implemented on WebHeaderCollection.

Doesn't this break applications by removing a public API? I'm not sure this is the right fix to the documentation problem.

Have you tried writing a .NET Core application and trying to use the string indexer as well as the int indexer? Does the string indexer fail in some way in an application?

@davidsh davidsh requested a review from stephentoub June 16, 2020 13:18
@stephentoub
Copy link
Member

Doesn't this break applications by removing a public API?

I think it's actually ok. It's definitely a strange situation, but it's not a source breaking change because recompiling will just bind to the base method of the same signature, and if it were going to be a binary breaking change, it'd already be breaking, because the newslot method doesn't exist in the actual implementation. The base type has the same method name, and the C# compiler emits calls to this in a way where the runtime will end up just calling the base method, even though this has a new slot, e.g. via:

callvirt instance string [System.Net.WebHeaderCollection]System.Net.WebHeaderCollection::get_Item(string)

or

call instance string [System.Net.WebHeaderCollection]System.Net.WebHeaderCollection::get_Item(string)

depending on the access pattern.

@terrajobst and @davidwrighton to correct me if I'm wrong.

@davidwrighton
Copy link
Member

I agree with @stephentoub's analysis. I would describe this as an odd way to fix a documentation bug, but a good way to make the reference assemblies more closely resemble what they support.

@aik-jahoda
Copy link
Contributor Author

It seems we don't have any mechanism to prevent inconsistency in the future as the ref assemblies are maintained manually.

However it looks like it shouldn't be a big issue to write a tool to validate/autogenerate the code/assembly to prevent such inconsistency. Do you know why there is no such tool in place? I think it definitely worth to simplify the process of maintaining any part of code.

@aik-jahoda
Copy link
Contributor Author

@davidsh, it seems the documentation stubs are generated from the ref assemblies. So I don't consider it as fixing the documentation, but the documentation point to ref assembly problem (inconsistency with our code base in this case).

Question is if the fix is correct otherwise I suppose wee need to add this indexer to the implementation code base.

@stephentoub
Copy link
Member

stephentoub commented Jun 17, 2020

It seems we don't have any mechanism to prevent inconsistency in the future as the ref assemblies are maintained manually.

We do; there's tooling that issues warnings/errors on such inconsistencies. For some reason this particular difference doesn't trigger it (probably related to the fact that you can use the method successfully because of the base method).
cc: @ericstj

@ericstj
Copy link
Member

ericstj commented Jun 17, 2020

This is considered a compatible change which is why APICompat doesn’t flag it.

@davidsh davidsh added this to the 5.0.0 milestone Jun 17, 2020
@aik-jahoda aik-jahoda merged commit bd1dbde into dotnet:master Jun 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants