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

FilePath and DirectoryPath implicit conversions should return null when passed null #2481

Closed
jnm2 opened this Issue Feb 19, 2019 · 9 comments

Comments

Projects
None yet
2 participants
@jnm2
Copy link
Contributor

commented Feb 19, 2019

Cake 0.30.0

This is the code I would like to write. It uses a conditional expression to keep it short and sweet:

        VSTest(testAssembly, new VSTestSettings
        {
            SettingsFile = keepWorkspaces ? "KeepWorkspaces.runsettings" : null
        });

If keepWorkspaces is false, that causes this error:

Value cannot be null.
Parameter name: path

Presumably SettingsFile is already null to start with, representing the option to not use a settings file, so null should be a valid option when setting the property.

As a workaround, I have to introduce two new statements and break the order that I'd prefer:

        var settings = new VSTestSettings();
        if (keepWorkspaces) settings.SettingsFile = "KeepWorkspaces.runsettings";
        VSTest(testAssembly, settings);
@devlead

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Is it VSTest or new VSTestSettings that gives you the error issue?

Maybe an helper method could be convenient, something like this (atm you could put it in a cake script):

public static T If<T>(this T value, Func<T, bool> predicate, Action<T> action)
{
    if (predicate == null || !predicate(value))
    {
        return value;
    }

    action?.Invoke(value);

    return value;
}

used like

new VSTestSettings{
}.If(_=>keepWorkspaces, settings => settings.SettingsFile = "KeepWorkspaces.runsettings");
@jnm2

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

It's hard to tell from the console output (https://ci.appveyor.com/project/CharliePoole/nunit3-vs-adapter/builds/22469410#L1339), but looking at https://github.com/cake-build/cake/blob/v0.30.0/src/Cake.Common/Tools/VSTest/VSTestSettings.cs#L19...

Edit: see the real explanation at #2481 (comment)

Ah, I just figured it out! The problem is that the implicit conversion operator from (string)null to FilePath is throwing an exception. I would have said that an implicit operator that throws is a contradiction in terms, since implicit operations are semantically supposed to be guaranteed to succeed at compile time just by knowing the types.

https://docs.microsoft.com/dotnet/csharp/language-reference/keywords/implicit:

> In general, implicit conversion operators should never throw exceptions and never lose information so that they can be used safely without the programmer's awareness. If a conversion operator cannot meet those criteria, it should be marked explicit.

Therefore, these operators should check to return null for null input:

public static implicit operator FilePath(string path)
{
return FromString(path);
}

public static implicit operator DirectoryPath(string path)
{
return FromString(path);
}

@jnm2

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

This is what is implied, according to the C# language:

VSTest(testAssembly, new VSTestSettings
{
    SettingsFile = (FilePath)(keepWorkspaces ? "KeepWorkspaces.runsettings" : null)
});

So my workaround for now will be:

VSTest(testAssembly, new VSTestSettings
{
    SettingsFile = keepWorkspaces ? (FilePath)"KeepWorkspaces.runsettings" : null
});
@devlead

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Agreed null should return null.

@jnm2

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

Are there implicit operators in Cake besides these two?

@devlead

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

A quick search gives these places

public static implicit operator FilePath(ConvertableFilePath path)
{
return path?.Path;
}

public static implicit operator DirectoryPath(ConvertableDirectoryPath path)
{
return path?.Path;
}

public static implicit operator ProcessArgumentBuilder(string value)
{
return FromString(value);
}

public static implicit operator DirectoryPath(string path)
{
return FromString(path);
}

@jnm2

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

So that makes three total places that should be fixed: the two above and then ProcessArgumentBuilder. Good enough to put up for grabs?

@devlead

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

@jnm2 sounds good to me.

@jnm2 jnm2 changed the title VSTestSettings.SettingsFile does not allow setting to null, preventing use of conditional expression FilePath and DirectoryPath implicit conversions throw for null Feb 19, 2019

@devlead devlead added the Bug label Feb 19, 2019

@devlead devlead added this to the v0.33.0 milestone Feb 19, 2019

@jnm2 jnm2 changed the title FilePath and DirectoryPath implicit conversions throw for null FilePath and DirectoryPath implicit conversions should return null when passed null Feb 20, 2019

@jnm2

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

It wasn't the operators that were throwing after all. What actually was happening was that new FilePath(null) was returned from the operator. new FilePath(null) is a time bomb that causes the ArgumentNullException to happen much later:

if (settings.SettingsFile != null)
{
builder.AppendSwitchQuoted("/Settings", ":", settings.SettingsFile.MakeAbsolute(_environment).FullPath);

public FilePath MakeAbsolute(ICakeEnvironment environment)
{
if (environment == null)
{
throw new ArgumentNullException(nameof(environment));
}
return IsRelative
? environment.WorkingDirectory.CombineWithFilePath(this).Collapse()

public FilePath CombineWithFilePath(FilePath path)
{
if (path == null)
{
throw new ArgumentNullException(nameof(path));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.