Deprecation handling - better error messages + compile-time error when time to deprecate #4912

jmarrec · 2023-06-06T22:44:35Z

Pull request overview

It's been really annoying tracking the deprecated methods. This PR aims to provide facilities to make it easier.

A new DEPRECATED_AT_MSG macro is added in core/DeprecatedHelpers.cpp for the CPP files
- This will automatically log a Warn to the registered logger, with the function name, the deprecated version, and an extra message
- This will issue a compile-time error otherwise
  - No way you can ignore it.
  - No need to exercise the method anywhere
OS_DEPRECATED (for the hpp files) now takes (major, minor, patch) at which it was deprecated so if you try to use that method in C++ you get a better deprecation warning such as 'void deprecatedFun()' is deprecated: Deprecated at 3.2.0. It will be removed after three releases. This also makes use of the [[deprecated()]] attribute instead of using attribute and __declspec

Given this test program:

struct A {
  OS_DEPRECATED(3, 5, 0) void myFunc() { DEPRECATED_AT_MSG(3, 5, 0, "Use XXX instead."); }
};
A a; A.myFunc("test");

While we're still within the same major version, and less than 3 minor versions from deprecation, this is the log message

[loggerChannel] <0>As of 3.5.0, void A::myFunc(const string&) is deprecated. Use XXX instead.

After that, a compile-time error is issued:

<source>: In member function 'void A::myFunc(const string&)':
<source>:98:17: error: static assertion failed: Time to remove, deprecated at 3.5.0
   98 |   static_assert(!openstudio::cx_VersionString{}.isGreaterThan(deprecatedAt),   \
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:106:39: note: in expansion of macro 'DEPRECATED_AT_MSG'
  106 |   void myFunc(const std::string& s) { DEPRECATED_AT_MSG(3, 5, 0, "Use XXX instead."); }
      |                                       ^~~~~~~~~~~~~~~~~
ASM generation compiler returned: 1

The deprecation warning when you try to use it from C++

<source>:185:11: warning: 'void A::myFunc(const string&)' is deprecated: Deprecated at 3.5.0. It will be removed after three releases [-Wdeprecated-declarations]
  185 |   a.myFunc("test");
      |   ~~~~~~~~^~~~~~~~
<source>:147:34: note: declared here
  147 |   OS_DEPRECATED_AT(3, 5, 0) void myFunc(const std::string& s) { DEPRECATED_AT_MSG(3, 5, 0, "Use XXX instead."); }
      |

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

…pile-time error when it's time to remove deprecated

…model) cd developer/ruby && ruby DeprecatedMethods.rb

…for better message

jmarrec · 2023-06-06T22:45:54Z

CMakeLists.txt

@@ -206,7 +206,7 @@ set(ENERGYPLUS_REPO "NREL")
 set(RADIANCE_VERSION "5.0.a.12")

 # configure file with version information
-configure_file(${PROJECT_SOURCE_DIR}/OpenStudio.in ${PROJECT_BINARY_DIR}/src/OpenStudio.hxx)
+configure_file(${PROJECT_SOURCE_DIR}/OpenStudio.hxx.in ${PROJECT_BINARY_DIR}/src/OpenStudio.hxx)


was annoyed this wasn't hxx.in so I changed it

jmarrec · 2023-06-06T22:46:50Z

OpenStudio.hxx.in

+namespace openstudio {
+namespace detail {
+
+inline constexpr int cx_openStudioVersionMajor() {
+  return ${PROJECT_VERSION_MAJOR};
+}
+inline constexpr int cx_openStudioVersionMinor() {
+  return ${PROJECT_VERSION_MINOR};
+}
+inline constexpr int cx_openStudioVersionPatch() {
+  return ${PROJECT_VERSION_PATCH};
+}
+inline constexpr std::string_view cx_openStudioVersion() {
+  return "${OPENSTUDIO_VERSION}";
+}
+
+}  // namespace detail
+}  // namespace openstudio


Nobody responded when I asked if we could change inline std::string openStudioVersionMajor() to return int or not, so I added this constexpr int version in a detail namespace

jmarrec · 2023-06-06T22:47:11Z

src/utilities/core/Deprecated.hpp

+#define OS_DEPRECATED(__deprecatedAtVersionMajor__, __deprecatedAtVersionMinor__, __deprecatedAtVersionPatch__)                   \
+  [[deprecated("Deprecated at " #__deprecatedAtVersionMajor__ "." #__deprecatedAtVersionMinor__ "." #__deprecatedAtVersionPatch__ \
+               ". It will be removed after three releases.")]]


OS_DEPRECATED changed here

jmarrec · 2023-06-06T22:48:36Z

src/utilities/core/DeprecatedHelpers.hpp

+#ifndef UTILITIES_CORE_DEPRECATEDHELPERS_HPP
+#define UTILITIES_CORE_DEPRECATEDHELPERS_HPP
+
+#include "Compare.hpp"
+#include "Exception.hpp"
+#include "Logger.hpp"
+
+#include <OpenStudio.hxx>
+
+#include <boost/current_function.hpp>
+#include <fmt/core.h>
+#include <fmt/compile.h>
+
+#include <string>
+
+namespace openstudio {
+
+struct cx_VersionString
+{
+  constexpr cx_VersionString(int t_major, int t_minor, int t_patch) : major(t_major), minor(t_minor), patch(t_patch) {}
+
+  constexpr cx_VersionString() = default;
+
+  constexpr bool isGreaterThan(const cx_VersionString& other, int numMinors = 3) const {
+    return (major > other.major) || (minor >= other.minor + numMinors);
+  }
+
+  std::string str() const {
+    return fmt::format("{}.{}.{}", major, minor, patch);
+  }
+
+  int major = openstudio::detail::cx_openStudioVersionMajor();
+  int minor = openstudio::detail::cx_openStudioVersionMinor();
+  int patch = openstudio::detail::cx_openStudioVersionPatch();
+};
+
+// static inline constexpr cx_VersionString cx_currentVersion{};
+
+namespace detail {
+  inline void log_deprecation_and_throw_if_time_to_remove(const std::string& deprecatedAt, const std::string& logChannel,
+                                                          const std::string& methodName, const std::string& extraMessage = "") {
+    if (extraMessage.empty()) {
+      LOG_FREE(Warn, logChannel, "As of " << deprecatedAt << ", " << methodName << " is deprecated.");
+    } else {
+      LOG_FREE(Warn, logChannel, "As of " << deprecatedAt << ", " << methodName << " is deprecated. " << extraMessage);
+    }
+  }
+}  // namespace detail
+}  // namespace openstudio
+
+#define DEPRECATED_AT(__deprecatedAtVersionMajor__, __deprecatedAtVersionMinor__, __deprecatedAtVersionPatch__) \
+  DEPRECATED_AT_MSG(__deprecatedAtVersionMajor__, __deprecatedAtVersionMinor__, __deprecatedAtVersionPatch__, "")
+
+#define DEPRECATED_AT_MSG(__deprecatedAtVersionMajor__, __deprecatedAtVersionMinor__, __deprecatedAtVersionPatch__, __message__)                     \
+  constexpr openstudio::cx_VersionString deprecatedAt(__deprecatedAtVersionMajor__, __deprecatedAtVersionMinor__, __deprecatedAtVersionPatch__);     \
+  static_assert(true || !openstudio::cx_VersionString{}.isGreaterThan(deprecatedAt),                                                                 \
+                "Time to remove, deprecated at " #__deprecatedAtVersionMajor__ "." #__deprecatedAtVersionMinor__ "." #__deprecatedAtVersionPatch__); \
+  openstudio::detail::log_deprecation_and_throw_if_time_to_remove(deprecatedAt.str(), logChannel(), BOOST_CURRENT_FUNCTION, __message__)
+
+#endif  // UTILITIES_CORE_DEPRECATEDHELPERS_HPP


DeprecatedHelpers for the CPP files. I didn't want to pollute the headers of the files that have deprecated methods with the deps, so I made it a separate file from Deprecated.hpp.

The rest is a mix of constexpr/consteval and macro hell.

jmarrec · 2023-06-06T22:49:59Z

src/utilities/core/DeprecatedHelpers.hpp

+
+#define DEPRECATED_AT_MSG(__deprecatedAtVersionMajor__, __deprecatedAtVersionMinor__, __deprecatedAtVersionPatch__, __message__)                     \
+  constexpr openstudio::cx_VersionString deprecatedAt(__deprecatedAtVersionMajor__, __deprecatedAtVersionMinor__, __deprecatedAtVersionPatch__);     \
+  static_assert(true || !openstudio::cx_VersionString{}.isGreaterThan(deprecatedAt),                                                                 \


I've disabled the static_assert for now, because there are tons of methods that due to deprecate and I wanted this to be reviewed before I remove them.

TODO: remove the disabling part

Suggested change

static_assert(true || !openstudio::cx_VersionString{}.isGreaterThan(deprecatedAt), \

static_assert(!openstudio::cx_VersionString{}.isGreaterThan(deprecatedAt), \

jmarrec · 2023-06-06T22:50:39Z

src/model/AirTerminalSingleDuctVAVReheat.cpp

@@ -825,7 +826,7 @@ namespace model {
  }

  std::string AirTerminalSingleDuctVAVReheat::zoneMinimumAirFlowMethod() {
-    LOG(Warn, "zoneMinimumAirFlowMethod is deprecated, please use zoneMinimumAirFlowInputMethod");
+    DEPRECATED_AT_MSG(3, 0, 1, "Use zoneMinimumAirFlowInputMethod instead.");


Example usage in the CPP file

jmarrec · 2023-06-06T22:50:50Z

src/model/AirTerminalSingleDuctVAVReheat.hpp

@@ -90,14 +90,14 @@ namespace model {
    /** Returns the value of the MaximumAirFlowRate field. */
    std::string zoneMinimumAirFlowInputMethod();
    /** deprecated **/
-    OS_DEPRECATED std::string zoneMinimumAirFlowMethod();
+    OS_DEPRECATED(3, 0, 1) std::string zoneMinimumAirFlowMethod();


Example usage in the HPP file

jmarrec · 2023-06-06T22:51:20Z

developer/ruby/deprecated_methods.csv

Updated the deprecated_methods.csv while I was at it.

jmarrec · 2023-06-06T22:55:19Z

src/utilities/core/DeprecatedHelpers.hpp

+  constexpr bool isGreaterThan(const cx_VersionString& other, int numMinors = 3) const {
+    return (major > other.major) || (minor >= other.minor + numMinors);
+  }


This is the logic I'm using to determine whether it's time to remove the deprecated method.

I have another way in mind that could capture the patch (if that's what we want), but it involves keeping a version history up to date, so not a fan.

As a free function:

consteval std::size_t numVersionsPassed( openstudio::cx_VersionString deprecatedAt) { std::array<openstudio::cx_VersionString, 3> versionHistory{{ {3, 5, 0}, {3, 6, 0}, {3, 6, 1}, }}; auto curVersion = openstudio::cx_VersionString{}; auto it = std::find(versionHistory.begin(), versionHistory.end(), deprecatedAt); auto itCur = std::find(versionHistory.begin(), versionHistory.end(), curVersion); if (it == versionHistory.end()) { // static_assert(false, "deprecatedAt not found in versionHistory"); } std::size_t offset = 0; if (itCur == versionHistory.end()) { offset = 1; } return std::distance(it, versionHistory.end()) - 1 + offset; }

This is the key thing that that needs to be maintained right?

If we wanted to capture patch, yeah; would have to keep updating versionHistory. Otherwise the maintenance is nil, just removing deprecated functions when we get the compile time error.

ci-commercialbuildings · 2023-06-06T23:35:59Z

CI Results for a1a3eef:

Ubuntu 20.04:
Ubuntu 22.04:
- OpenStudio-3.7.0-alpha+a1a3eefe32-Ubuntu-22.04-x86_64.deb
- OpenStudio-3.7.0-alpha+a1a3eefe32-Ubuntu-22.04-x86_64.tar.gz
Darwin:
- OpenStudio-3.7.0-alpha+a1a3eefe32-Darwin-x86_64.dmg
- OpenStudio-3.7.0-alpha+a1a3eefe32-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.7.0-alpha+a1a3eefe32-Windows.exe
- OpenStudio-3.7.0-alpha+a1a3eefe32-Windows.tar.gz
Code Coverage:

kbenne

I think this is cool. I still hope we deprecate sparingly.

jmarrec added 5 commits June 6, 2023 21:09

Rename OpenStudio.in to OpenStudio.hxx.in for clarity

09e0c40

Add deprecated helpers + use it in a couple of files

bfadb32

Change to a static_assert with a custom error message so we get a com…

e74e664

…pile-time error when it's time to remove deprecated

Grab all the other deprecated methods (including recent / outside of …

d853727

…model) cd developer/ruby && ruby DeprecatedMethods.rb

Change OS_DEPRECATED to take version info at which it was deprecated …

a1a3eef

…for better message

jmarrec added Developer Issue Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Jun 6, 2023

jmarrec self-assigned this Jun 6, 2023

jmarrec requested review from tijcolem and kbenne June 6, 2023 22:45

jmarrec commented Jun 6, 2023

View reviewed changes

jmarrec requested a review from joseph-robertson June 6, 2023 22:51

jmarrec commented Jun 6, 2023

View reviewed changes

kbenne approved these changes Jun 7, 2023

View reviewed changes

tijcolem approved these changes Jun 20, 2023

View reviewed changes

jmarrec merged commit a84c101 into develop Jun 29, 2023
5 of 6 checks passed

jmarrec deleted the deprecation_handling branch June 29, 2023 14:28

joseph-robertson mentioned this pull request Jul 13, 2023

AirLoopHVACUnitarySystem set Method During XXX Operation - option #4905

Merged

19 tasks

jmarrec added a commit that referenced this pull request Feb 23, 2024

Doxygen config for OS_DEPRECATED changes from #4912

2973af0

wenyikuang pushed a commit that referenced this pull request Feb 23, 2024

Doxygen config for OS_DEPRECATED changes from #4912 (#5097)

80147ba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecation handling - better error messages + compile-time error when time to deprecate #4912

Deprecation handling - better error messages + compile-time error when time to deprecate #4912

jmarrec commented Jun 6, 2023 •

edited by joseph-robertson

jmarrec Jun 6, 2023

jmarrec Jun 6, 2023

jmarrec Jun 6, 2023

jmarrec Jun 6, 2023

jmarrec Jun 6, 2023

jmarrec Jun 6, 2023

jmarrec Jun 6, 2023

jmarrec Jun 6, 2023

jmarrec Jun 6, 2023

kbenne Jun 7, 2023

jmarrec Jun 12, 2023

ci-commercialbuildings commented Jun 6, 2023 •

edited

kbenne left a comment

	static_assert(true \|\| !openstudio::cx_VersionString{}.isGreaterThan(deprecatedAt), \
	static_assert(!openstudio::cx_VersionString{}.isGreaterThan(deprecatedAt), \

Deprecation handling - better error messages + compile-time error when time to deprecate #4912

Deprecation handling - better error messages + compile-time error when time to deprecate #4912

Conversation

jmarrec commented Jun 6, 2023 • edited by joseph-robertson

Pull request overview

Pull Request Author

Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Jun 6, 2023 • edited

kbenne left a comment

Choose a reason for hiding this comment

jmarrec commented Jun 6, 2023 •

edited by joseph-robertson

ci-commercialbuildings commented Jun 6, 2023 •

edited