Skip to content

Commit

Permalink
Fix lazy string format (#5924)
Browse files Browse the repository at this point in the history
  • Loading branch information
rokonec authored Dec 4, 2020
1 parent f711132 commit 7cc83da
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 80 deletions.
39 changes: 0 additions & 39 deletions src/Framework/LazyFormattedBuildEventArgs.cs
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
// 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
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
9 changes: 8 additions & 1 deletion src/Shared/TaskLoggingHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,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 formatted using given arguments
ResourceUtilities.FormatString(message, messageArgs);
}
#endif

BuildMessageEventArgs e = new BuildMessageEventArgs
(
Expand Down Expand Up @@ -463,7 +470,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
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

0 comments on commit 7cc83da

Please sign in to comment.