Skip to content

[BUG] Non-empty line after one-liner type with requires clause fails assertion #472

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

Closed
JohelEGP opened this issue May 25, 2023 · 6 comments · Fixed by #528
Closed

[BUG] Non-empty line after one-liner type with requires clause fails assertion #472

JohelEGP opened this issue May 25, 2023 · 6 comments · Fixed by #528
Labels
bug Something isn't working

Comments

@JohelEGP
Copy link
Contributor

Title: Non-empty line after one-liner type with requires clause fails assertion.

Minimal reproducer (https://cpp2.godbolt.org/z/bfh5bfTj4):

t: type requires true = { }
main: () = { }
Commands:
cppfront -clean-cpp1 main.cpp2

Actual result and error:

Output.

cppfront: source/cppfront.cpp:523: void cpp2::positional_printer::align_to(cpp2::source_position): Assertion `psource && 0 <= curr_pos.lineno && curr_pos.lineno < std::ssize(psource->get_lines())+1' failed.
@JohelEGP JohelEGP added the bug Something isn't working label May 25, 2023
@filipsajdak
Copy link
Contributor

I have an easy fix for the assertion:

diff --git a/source/cppfront.cpp b/source/cppfront.cpp
index 5b89964..44cc52e 100644
--- a/source/cppfront.cpp
+++ b/source/cppfront.cpp
@@ -5094,7 +5094,7 @@ public:
                 if (n.requires_clause_expression) {
                     printer.print_cpp2("requires( ", n.requires_pos);
                     emit(*n.requires_clause_expression);
-                    printer.print_cpp2(" )\n\n", n.requires_pos);
+                    printer.print_cpp2(" )\n", n.requires_pos);
                 }
 
                 printer.print_cpp2("class ", n.position());

But even without the parsing error, the result cpp1 code is not good (check here: https://cpp2.godbolt.org/z/eGMMz8Txj)

#include "cpp2util.h"

requires( true )

class t;
requires( true )

class t {
      public: t() = default;
      public: t(t const&) = delete; /* No 'that' constructor, suppress copy */
      public: auto operator=(t const&) -> void = delete;

};

auto main() -> int;

auto main() -> int{}

error:

build/_cppfront/main.cpp:5:1: error: expected unqualified-id before 'requires'
    5 | requires( true )
      | ^~~~~~~~
build/_cppfront/main.cpp:8:1: error: expected unqualified-id before 'requires'
    8 | requires( true )
      | ^~~~~~~~
make[2]: *** [CMakeFiles/test.dir/build.make:81: CMakeFiles/test.dir/_cppfront/main.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/test.dir/all] Error 2

@JohelEGP
Copy link
Contributor Author

Hmmm... yes.
It's because t isn't a templated entity.
Just add a template-head to it.
Thank you for looking into this!

@filipsajdak
Copy link
Contributor

The most probably requires clause for non-templates should be a cpp2 parsing error.

@JohelEGP
Copy link
Contributor Author

That's actually a Cpp1 semantic rule
that could be elevated to a Cpp2 parser error,
but so could many other Cpp1 semantic rules.

@hsutter
Copy link
Owner

hsutter commented Aug 10, 2023

Thanks!
I think I'd prefer leaving the semantic rule enforced by the Cpp1 compiler, as long as it's reasonably readable and points to the right Cpp2 line number.

@hsutter
Copy link
Owner

hsutter commented Aug 11, 2023

Re elevating the Cpp1 semantic rule to Cpp2, repeating rationale from #528 for visibility:

I do think there's value in diagnosing a requires on a non-templated type for type declarations.

Rationale:

  • writing a stray requires on a function already gives a nice Cpp1 diagnostic (e.g., MSVC says error C7599: 't::f': a trailing requires clause is only allowed on a templated function)
  • writing a stray requires on a type currently lowers it to a naked requires at global scope before the type which leads to a harder-to-understand Cpp1 diagnostic (e.g., MSVC says error C2059: syntax error: 'requires', which is correct and indicates the rough issue, but it's not as specific)

So diagnosing the type case in Cpp2 delivers the primary diagnostic usability improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants