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

CS8600 removal #12993

Merged
merged 3 commits into from
May 16, 2024
Merged

CS8600 removal #12993

merged 3 commits into from
May 16, 2024

Conversation

csiki2
Copy link
Collaborator

@csiki2 csiki2 commented May 7, 2024

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/nullable-warnings?f1url=%3FappId%3Droslyn%26k%3Dk(CS8600)

Converting null literal or possible null value to non-nullable type.

We can safely change the return value to string since the only caller just uses IsNullOrEmpty, don't differentiate between the two state (null vs empty string).

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/nullable-warnings?f1url=%3FappId%3Droslyn%26k%3Dk(CS8600)

Converting null literal or possible null value to non-nullable type.

We can safely change the return value to string since the only caller just uses IsNullOrEmpty, don't differentiate between the two state (null vs empty string).
@csiki2 csiki2 requested review from soosr and molnard May 7, 2024 13:16
@csiki2
Copy link
Collaborator Author

csiki2 commented May 7, 2024

The Required part can be fixed by #12989

@kiminuo
Copy link
Collaborator

kiminuo commented May 12, 2024

It looks to me that one can do 84079dc and given that there is only one caller, it should be safe. Could you check?

{
return "";
}

Copy link
Collaborator Author

@csiki2 csiki2 May 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that we can do this?
Due to how it is used:

	private string PreComposeText(string input)
	{
		input = RemoveInvalidCharacters(input);
		var preComposedText = Text ?? "";
		var caretIndex = CaretIndex;
		var selectionStart = SelectionStart;
		var selectionEnd = SelectionEnd;

		if (!string.IsNullOrEmpty(input) && (MaxLength == 0 ||

It's a bit suspicious.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked it and input looks non-null.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or can you see where it can become null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me string.IsNullOrEmpty implies that it might be null, or was it there for no reason?

Copy link
Collaborator Author

@csiki2 csiki2 May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently only used here:

protected override void OnTextInput(TextInputEventArgs e)
{
	var input = e.Text == null ? "" : e.Text.TotalTrim();

You are right, this is never null. The code is simply misleading.

@kiminuo kiminuo merged commit bd3a6d7 into WalletWasabi:master May 16, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants