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.

Sign up for GitHub

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

Jump to bottom

Fix lazy string format #5924

Merged
merged 2 commits into from
Dec 4, 2020
Merged

Fix lazy string format #5924

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36adfca
Fix lazy string format
rokonec Dec 1, 2020
4b49de7
Review changes - ladipro #1
rokonec Dec 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 0 additions & 39 deletions src/Framework/LazyFormattedBuildEventArgs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,45 +199,6 @@ private static string FormatString(CultureInfo culture, string unformatted, para
if ((args?.Length > 0))
{
#if DEBUG

#if VALIDATERESOURCESTRINGS
rokonec marked this conversation as resolved.
Show resolved Hide resolved
// The code below reveals many places in our codebase where
// we're not using all of the data given to us to format
// strings -- but there are too many to presently fix.
// Rather than toss away the code, we should later build it
// and fix each offending resource (or the code processing
// the resource).

// String.Format() will throw a FormatException if args does
// not have enough elements to match each format parameter.
// However, it provides no feedback in the case when args contains
// more elements than necessary to replace each format
// parameter. We'd like to know if we're providing too much
// data in cases like these, so we'll fail if this code runs.

// We create an array with one fewer element
object[] trimmedArgs = new object[args.Length - 1];
Array.Copy(args, 0, trimmedArgs, 0, args.Length - 1);

bool caughtFormatException = false;
try
{
// This will throw if there aren't enough elements in trimmedArgs...
String.Format(CultureInfo.CurrentCulture, unformatted, trimmedArgs);
}
catch (FormatException)
{
caughtFormatException = true;
}

// If we didn't catch an exception above, then some of the elements
// of args were unnecessary when formatting unformatted...
Debug.Assert
(
caughtFormatException,
String.Format("The provided format string '{0}' had fewer format parameters than the number of format args, '{1}'.", unformatted, args.Length)
);
#endif
// If you accidentally pass some random type in that can't be converted to a string,
// FormatResourceString calls ToString() which returns the full name of the type!
foreach (object param in args)
Expand Down
39 changes: 0 additions & 39 deletions src/Shared/ResourceUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,45 +227,6 @@ internal static string FormatString(string unformatted, params object[] args)
if ((args?.Length > 0))
{
#if DEBUG

#if VALIDATERESOURCESTRINGS
// The code below reveals many places in our codebase where
// we're not using all of the data given to us to format
// strings -- but there are too many to presently fix.
// Rather than toss away the code, we should later build it
// and fix each offending resource (or the code processing
// the resource).

// String.Format() will throw a FormatException if args does
// not have enough elements to match each format parameter.
// However, it provides no feedback in the case when args contains
// more elements than necessary to replace each format
// parameter. We'd like to know if we're providing too much
// data in cases like these, so we'll fail if this code runs.

// We create an array with one fewer element
object[] trimmedArgs = new object[args.Length - 1];
Array.Copy(args, 0, trimmedArgs, 0, args.Length - 1);

bool caughtFormatException = false;
try
{
// This will throw if there aren't enough elements in trimmedArgs...
String.Format(CultureInfo.CurrentCulture, unformatted, trimmedArgs);
}
catch (FormatException)
{
caughtFormatException = true;
}

// If we didn't catch an exception above, then some of the elements
// of args were unnecessary when formatting unformatted...
Debug.Assert
(
caughtFormatException,
String.Format("The provided format string '{0}' had fewer format parameters than the number of format args, '{1}'.", unformatted, args.Length)
);
#endif
// If you accidentally pass some random type in that can't be converted to a string,
// FormatResourceString calls ToString() which returns the full name of the type!
foreach (object param in args)
Expand Down
26 changes: 16 additions & 10 deletions src/Shared/TaskLoggingHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,7 @@ public string ExtractMessageCode(string message, out string messageWithoutCodePr
/// <exception cref="InvalidOperationException">Thrown when the <c>TaskResources</c> property of the owner task is not set.</exception>
public virtual string FormatResourceString(string resourceName, params object[] args)
{
ErrorUtilities.VerifyThrowArgumentNull(resourceName, nameof(resourceName));
ErrorUtilities.VerifyThrowInvalidOperation(TaskResources != null, "Shared.TaskResourcesNotRegistered", TaskName);

string resourceString = TaskResources.GetString(resourceName, CultureInfo.CurrentUICulture);

ErrorUtilities.VerifyThrowArgument(resourceString != null, "Shared.TaskResourceNotFound", resourceName, TaskName);

return FormatString(resourceString, args);
return FormatString(GetResourceMessage(resourceName), args);
}

/// <summary>
Expand All @@ -232,7 +225,13 @@ public virtual string FormatString(string unformatted, params object[] args)
/// <returns>The message from resource.</returns>
public virtual string GetResourceMessage(string resourceName)
{
string resourceString = FormatResourceString(resourceName, null);
ErrorUtilities.VerifyThrowArgumentNull(resourceName, nameof(resourceName));
ErrorUtilities.VerifyThrowInvalidOperation(TaskResources != null, "Shared.TaskResourcesNotRegistered", TaskName);

string resourceString = TaskResources.GetString(resourceName, CultureInfo.CurrentUICulture);

ErrorUtilities.VerifyThrowArgument(resourceString != null, "Shared.TaskResourceNotFound", resourceName, TaskName);

return resourceString;
}
#endregion
Expand Down Expand Up @@ -272,6 +271,13 @@ public void LogMessage(MessageImportance importance, string message, params obje
{
// No lock needed, as BuildEngine methods from v4.5 onwards are thread safe.
ErrorUtilities.VerifyThrowArgumentNull(message, nameof(message));
#if DEBUG
if (messageArgs?.Length > 0)
{
// Verify that message can be formated using given arguments
rokonec marked this conversation as resolved.
Show resolved Hide resolved
ResourceUtilities.FormatString(message, messageArgs);
}
#endif

BuildMessageEventArgs e = new BuildMessageEventArgs
(
Expand Down Expand Up @@ -463,7 +469,7 @@ public void LogMessageFromResources(MessageImportance importance, string message
// global state.
ErrorUtilities.VerifyThrowArgumentNull(messageResourceName, nameof(messageResourceName));

LogMessage(importance, FormatResourceString(messageResourceName, messageArgs));
LogMessage(importance, GetResourceMessage(messageResourceName), messageArgs);
#if DEBUG
// Assert that the message does not contain an error code. Only errors and warnings
// should have error codes.
Expand Down
2 changes: 1 addition & 1 deletion src/Utilities/TrackedDependencies/FileTracker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ internal static void LogMessageFromResources(TaskLoggingHelper Log, MessageImpor
{
ErrorUtilities.VerifyThrowArgumentNull(messageResourceName, nameof(messageResourceName));

Log.LogMessage(importance, AssemblyResources.FormatResourceString(messageResourceName, messageArgs));
Log.LogMessage(importance, AssemblyResources.GetString(messageResourceName), messageArgs);
}
}

Expand Down