-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Annotate ObjectPool with nullable #22823
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
Conversation
49a9bf2
to
42963fb
Compare
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
@@ -13,6 +13,7 @@ public partial class DefaultObjectPool<T> : Microsoft.Extensions.ObjectPool.Obje | |||
{ | |||
public DefaultObjectPool(Microsoft.Extensions.ObjectPool.IPooledObjectPolicy<T> policy) { } | |||
public DefaultObjectPool(Microsoft.Extensions.ObjectPool.IPooledObjectPolicy<T> policy, int maximumRetained) { } | |||
[return: System.Diagnostics.CodeAnalysis.NotNullAttribute] | |||
public override T Get() { throw null; } | |||
public override void Return(T obj) { } |
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.
Can null be passed to Return? If T is not null everywhere then the notnull generic constraint could be used.
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.
T
already has a class
constraint. The compiler requires that both notnull
and class
to be the first generic constraint so you're unable to apply both simultaneously. That said, there aren't any null checks here. You could return a null value, but that would likely corrupt the pool
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.
As I discovered, T : class
indicates that T
is non-null. This removes the need to specify a NotNullAttribute, and we get the right behavior
@@ -13,6 +13,7 @@ public partial class DefaultObjectPool<T> : Microsoft.Extensions.ObjectPool.Obje | |||
{ | |||
public DefaultObjectPool(Microsoft.Extensions.ObjectPool.IPooledObjectPolicy<T> policy) { } | |||
public DefaultObjectPool(Microsoft.Extensions.ObjectPool.IPooledObjectPolicy<T> policy, int maximumRetained) { } | |||
[return: System.Diagnostics.CodeAnalysis.NotNullAttribute] |
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 NotNull also be on the base class? Would it ever make sense to return null from ObjectPool?
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.
It's already on the base class 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 see it -
aspnetcore/src/ObjectPool/ref/Microsoft.Extensions.ObjectPool.netcoreapp.cs
Lines 57 to 62 in 42963fb
public abstract partial class ObjectPool<T> where T : class | |
{ | |
protected ObjectPool() { } | |
public abstract T Get(); | |
public abstract void Return(T obj); | |
} |
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.
Oops, I think I meant to put this on the base type.
Are all the other critical APIs done? |
Hello @pranavkm! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
@JamesNK could I sign off on this? |
Double approved for extra effectiveness. |
Contributes to #5680