Everyone’s talking about it, so why should I not add something too.

Interview question

ChatGPT is pretty amazing. I asked it an interview question, and it just plain solved it. Well, there was one real world imperfection. I asked it to fix that problem, and then it went off the rails. The more I hinted, the worse the code got.

I won’t give the details here, because I want to keep using the question / topic.

Finding bugs

So I wondered if it could find bugs. First an easy one:

Q:

What's the problem with this C++ code:
int* foo() {
  auto ret = new int;
  int r2 = *ret * 2;
  return &r2;
}

A:
There are several problems with the code you provided:

1. The function foo is returning a pointer to a local variable r2,
   which is not safe because the memory for r2 is deallocated when the
   function returns. This can cause undefined behavior when you try to
   access the returned pointer after the function has returned.
2. The function foo is allocating memory for an integer with new int,
   but it is not deallocating that memory before the function
   returns. This can lead to a memory leak, as the allocated memory
   will remain allocated until the program ends.
3. The function does not have a way to handle the case where the
   memory allocation with new int fails. In this case, new int will
   throw an exception, which will not be caught by the function. This
   can cause the program to terminate unexpectedly.

To fix these problems, you could consider the following changes:

1. Instead of returning a pointer to a local variable, you could
   return a pointer to the dynamically allocated memory. This will
   ensure that the returned pointer remains valid after the function
   returns.
2. You should deallocate the memory that was allocated with new int
   when it is no longer needed. This can be done using the delete
   operator, like this: delete ret;.
3. You should catch any exceptions that might be thrown by new int and
   handle them appropriately. This can be done using a try-catch
   block, like this:

try {
  auto ret = new int;
  // ...
} catch (std::bad_alloc& e) {
  // Handle the exception here
}

That’s… pretty amazing. It missed that the value is uninitialized, and I’d argue with the last point, since an overall strategy for OOM is not something for just one little function to solve, but not bad.

How about a tricky one?

Let’s take my safe integer conversion tool from a previous post.

Q:
Is there anything wrong with this C++ code:

template<typename To, typename From>
std::optional<To> cast_int(const From from)
{
  // If casting from signed to unsigned then reject negative inputs.
  if (std::is_signed_v<From> && std::is_unsigned_v<To> && from < 0) {
    return {};
  }

  const To to = static_cast<To>(from);

  // If casting from unsigned to signed then the result must be positive.
  if (std::is_unsigned_v<From> && std::is_signed_v<To> && to < 0) {
    return {};
  }

  // If the number fits then it'll be the same number when cast back.
  if (from != static_cast<From>(to)) {
    return {};
  }
  return to;
}

A:
There are a few potential problems with the code you provided:

1. The function uses the std::optional type to represent a value that
   may or may not be present. However, std::optional is not available
   until C++17, so the code will not compile with earlier versions of
   C++.
2. The function uses the std::is_signed_v and std::is_unsigned_v type
   traits to check the signedness and unsignedness of the From and To
   template arguments. However, these type traits are not available
   until C++20, so the code will not compile with earlier versions of
   C++.
3. The function checks whether the value of from is negative before
   casting it to an unsigned type. However, this check will not catch
   all cases where the value of from is too large to fit in the target
   type. For example, if From is int and To is unsigned short, the
   value of from could be 65537, which is too large to fit in an
   unsigned short, but it is not negative.

To fix these problems, you could consider the following changes:

1. If you need to support earlier versions of C++, you could use a
   different mechanism to represent optional values. For example, you
   could use a std::pair<To, bool> where the second element is a
   boolean indicating whether the value is present or not.
2. If you need to support earlier versions of C++, you could use a
   different mechanism to check the signedness and unsignedness of the
   template arguments. For example, you could use the std::is_signed
   and std::is_unsigned type traits, which are available in C++11 and
   later.
3. To catch all cases where the value of from is too large to fit in
   the target type, you could use the std::numeric_limits class to
   check the maximum and minimum values that can be represented by the
   target type. For example:

if (from > std::numeric_limits<To>::max()) {
  return {};
}

if (from < std::numeric_limits<To>::min()) {
  return {};
}

Again, I’m impressed. It’s wrong, but I’m impressed. The first two comments are technically correct, but they are not actually problems. Or at least I would not call them problems.

The third one, though, is wrong. It’ll fail to convert from unsigned 0 to int.

template <typename To, typename From>
std::optional<To> cast_int(const From from)
{
  if (from > std::numeric_limits<To>::max()) {
    std::cerr << "Too big: " << from << "\n";
    return {};
  }

  if (from < std::numeric_limits<To>::min()) {
    std::cerr << "Too small: " << from
              << " < " << std::numeric_limits<To>::min() << "\n";
    return {};
  }
  return static_cast<To>(from);
}
int main()
{
  std::cout << cast_int<int>(unsigned(0)).value() << "\n";
}

This produces output:

Too small: 0 < -2147483648
0

Let’s follow up:

Q: The third suggestion would fail a conversion into type int, because
   of integer promotion rules.

A:
You are correct that the third suggestion I provided would not catch
all cases where the value of from is too large to fit in the target
type.

Integer promotion is a form of type coercion that occurs in C++ when
an expression involving integers of small types is used in a context
where an integer of a larger type is expected. During integer
promotion, the value of the small integer is first converted to a
larger integer type, and then the operation is performed using the
larger type. This means that if from is an integer of a small type
(e.g. char, short) and To is a larger integer type (e.g. int, long
long), then the value of from will be promoted to the larger type
before the comparison with std::numeric_limits<To>::max() or
std::numeric_limits<To>::min() is performed.

To catch all cases where the value of from is too large to fit in the
target type, you can use a combination of integer promotion and the
std::numeric_limits class. Here is an example of how you can do this:

if (from > static_cast<decltype(from)>(std::numeric_limits<To>::max())) {
  return {};
}

if (from < static_cast<decltype(from)>(std::numeric_limits<To>::min())) {
  return {};
}

This code first converts the maximum and minimum values of the target
type to the type of from using static_cast, and then compares the
value of from to these converted values. This ensures that the
comparison is performed using the correct types, and that integer
promotion is correctly applied.

And like with my interview followups, this is where it gets off the rails.

First of all the type of from is already available, it’s From. Second, casting min and max of the type To into From is the root problem, and you can’t just cast and expect it to work.

I guess ChatGPT has not read my blog post. :-)

Q: You can't just cast from To to From. If the signedness doesn't
   match then it may wrap at best, or undefined behavior at worst.

A:
You are correct that simply casting from To to From is not sufficient
to properly check whether the value of from is within the range of the
target type To.

If the signedness of From and To do not match, then a simple cast from
To to From can lead to undefined behavior, as you mentioned. This is
because casting a negative value to an unsigned type is undefined
behavior in C++.

To properly check whether the value of from is within the range of the
target type To, you can use a combination of integer promotion and the
std::numeric_limits class, as I suggested in my previous
response. This will ensure that the comparison is performed using the
correct types, and that integer promotion is correctly applied.

Alternatively, you can use the std::is_signed_v and std::is_unsigned_v
type traits to check the signedness and unsignedness of From and To,
and use different logic depending on the signedness of the types. For
example:

if constexpr (std::is_signed_v<From> == std::is_signed_v<To>) {
  // Both types have the same signedness
  if (from > std::numeric_limits<To>::max()) {
    return {};
  }

  if (from < std::numeric_limits<To>::min()) {
    return {};
  }
} else if constexpr (std::is_signed_v<From>) {
  // From is signed, To is unsigned
  if (from < 0) {
    return {};
  }

  if (from > static_cast<decltype(from)>(std::numeric_limits<To>::max())) {
    return {};
  }
} else {
  // From is unsigned, To is signed
  if (from > static_cast<decltype(from)>(std::numeric_limits<To>::max())) {
    return {};
  }
}

This code uses different logic depending on the signedness of the
types, which ensures that the correct comparisons are performed and
that integer promotion is correctly applied.

Wow, that is… almost correct. The static casts to decltypes need to be removed, to instead rely on integer promotions. Otherwise e.g. converting 0 in a signed 32bit int into an unsigned 64bit int will fail. The unsigned 64bit max will be converted into a signed 32bit int, and get the bit pattern for -1.

But damn, after that I think it’s correct.

Q: The decltype casts cause a bug with that solution.

A:
You are correct that the use of decltype in the code I provided can
cause a bug when the value of from is too large to fit in the target
type To.

The problem occurs because decltype(from) is the type of from, which
may be larger than the target type To. This means that when the
maximum or minimum value of To is converted to the type of from, the
result may be larger than the maximum or minimum value of To,
respectively. This can cause the comparison to always evaluate to
false, even if the value of from is within the range of To.

To fix this problem, you can use a different mechanism to ensure that
the comparison is performed using the correct types. One option is to
use a helper function that compares the value of from to the maximum
and minimum values of To using the correct types. Here is an example
of how you could do this:

template <typename From, typename To>
bool in_range(From from) {
  using common_type = std::common_type_t<From, To>;
  return static_cast<common_type>(from) >= static_cast<common_type>(std::numeric_limits<To>::min()) &&
         static_cast<common_type>(from) <= static_cast<common_type>(std::numeric_limits<To>::max());
}

template<typename To, typename From>
std::optional<To> cast_int(const From from)
{
  // If casting from signed to unsigned then reject negative inputs.
  if (std::is_signed_v<From> && std::is_unsigned_v<To> && from < 0) {
    return {};
  }

  const To to = static_cast<To>(from);

  // If casting from unsigned to signed then the result must be positive.
  if (std::is_unsigned_v<From> && std::is_signed_v<To> && to < 0) {
    return {};
  }

  // If the number fits then it'll be the same number when cast back.
  if (!in_range<From, To>(from)) {
    return {};
  }
  return to;
}

This code uses the std::common_type type trait to determine the common
type of From and To, and then uses static_cast to convert both from
and the minimum and maximum values of To to this common type. This
ensures that the comparison is performed using the correct types, and
that integer promotion is correctly applied.

Again, I’m impressed. But again, it’s wrong! common_type for int32_t and uint32_t will (just like integer promotion rules) become uint32_t, and the comparison will reject good input such as this:

std::cout << cast_int<int32_t>(uint32_t(0)).value() << "\n";

Conclusion

ChatGPT is really impressive. I’m not at risk of getting replaced yet, but for code challenges I think it does a much better job than most programmers. And certainly much faster.

But if it gets it slightly wrong then it seems unable to fix it.

For job security in the future as this gets better you should probably focus on turning business value into what to solve, not perfecting implementation. Because in the future the implementation will just be telling the computer to “add the ability for customers to upload their own profile photos via the web or the API, deduplicate and store them in GCS/S3. Write a blog post about it”.

In writing this I’ve gone back and forth about if I’m in danger in the next few years. But considering the amount of time I spend in meetings and on problem anticipation as opposed to implementation, maybe I have that to lean back on?

I guess until my employer trains on our code base and our specific customer needs. Then I’m fucked.