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 bad Debug.Asserts in DataGridView #308

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@hughbe
Copy link
Contributor

hughbe commented Jan 9, 2019

All can be triggered from tests in https://github.com/hughbe/winforms/tree/ongoing

@hughbe hughbe requested a review from dotnet/dotnet-winforms as a code owner Jan 9, 2019

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 9, 2019

@hughbe what are your feelings on replacing these with input validation and exceptions? After talking to Adam, we're in agreement that this is a stronger replacement here than deleting them

@hughbe

This comment has been minimized.

Copy link
Contributor

hughbe commented Jan 10, 2019

I didn't want to delete anything because that could break existing customers - it was my impression that compatibility takes precedence over these sorts of things. I will comment where throwing an exception instead would possibly break things

@@ -657,7 +657,6 @@ public virtual DataGridViewTriState Resizable
}
else
{
Debug.Assert(value == DataGridViewTriState.False, "TriState only supports NotSet, True, False");

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

This could be fixed by throwing an InvalidEnumArgumentException if value is not defined. The old behaviour would be to Resizable to false if invalid

@@ -996,10 +996,6 @@ public virtual bool Visible
{
switch (dataGridViewAdvancedBorderStyleInput.All)
{
case DataGridViewAdvancedCellBorderStyle.OutsetPartial:

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

We could throw here - not sure what though. I would be tempted to keep the old behaviour of simply bailing out for an unknown or supported dataGridViewAdvancedBorderStyleInput.All type

@@ -1803,7 +1799,6 @@ public object GetEditedFormattedValue(int rowIndex, DataGridViewDataErrorContext

internal Rectangle GetErrorIconBounds(int rowIndex)
{
Debug.Assert(this.DataGridView != null);

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Nothing to action on here, an exception is already thrown from GetInheritedStyle:

            if (this.DataGridView == null)
            {
                throw new InvalidOperationException(string.Format(SR.DataGridView_CellNeedsDataGridViewForInheritedStyle));
            }
@@ -275,9 +275,6 @@ public void PaintContent(Rectangle clipBounds)
DataGridViewAdvancedBorderStyle advancedBorderStyle,
DataGridViewPaintParts paintParts)
{
Debug.Assert(graphics != null);

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

The old behaviour here was to simply do nothing if graphics is null or throw a NullReferenceException so removing these without changes seems fine.

Alternatively, we could throw an ArgumentNullException in DataGridView.PaintCells and DataGridView.PaintHeader if graphics is null. But this has compat concerns as this would throw for null graphics with empty clip bounds where this previously didnt

@@ -314,8 +314,6 @@ public override DataGridViewCellStyle GetInheritedStyle(DataGridViewCellStyle in
throw new ArgumentOutOfRangeException(nameof(rowIndex));
}

Debug.Assert(this.DataGridView != null);

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

This could be changed to throw an ArgumentNullException. The old behaviour was to throw a NullReferenceException below

@@ -260,9 +260,6 @@ public void PaintHeader(DataGridViewPaintParts paintParts)
bool isFirstDisplayedRow,
bool isLastVisibleRow)
{
Debug.Assert(graphics != null);

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

The old behaviour here was to simply do nothing if graphics is null or throw a NullReferenceException so removing these without changes seems fine.

Alternatively, we could throw an ArgumentNullException in DataGridView.PaintCells and DataGridView.PaintHeader if graphics is null. But this has compat concerns as this would throw for null graphics with empty clip bounds where this previously didnt

@@ -279,9 +279,6 @@ public void PaintHeader(DataGridViewPaintParts paintParts)
bool isFirstDisplayedRow,
bool isLastVisibleRow)
{
Debug.Assert(graphics != null);

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

The old behaviour here was to simply do nothing if graphics is null or throw a NullReferenceException so removing these without changes seems fine.

Alternatively, we could throw an ArgumentNullException in DataGridView.PaintCells and DataGridView.PaintHeader if graphics is null. But this has compat concerns as this would throw for null graphics with empty clip bounds where this previously didnt

@hughbe

This comment has been minimized.

Copy link
Contributor

hughbe commented Jan 10, 2019

@zsd4yr also we need to investigate a policy for the following class of bugs: lack of parameter validation. For example, if a param is null or is invalid and this causes a NullReferenceException, is it appropriate to change this to an ArgumentNullException or an InvalidOperationException where applicable? Not sure what the compat burden is for this

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 10, 2019

I'm going to loop @Tanya-Solyanik in here as she may be able to speak better than I to where we could make these changes for safety even as they risk compat; for example, is it really valid to send null graphics?

@Tanya-Solyanik

This comment has been minimized.

Copy link
Member

Tanya-Solyanik commented Jan 14, 2019

It might be risky to add new exception to the existing code because someone might be already handling them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment