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.

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

[BUG] [GCC] UFCS with qualified function fails #746

Closed
JohelEGP opened this issue Oct 12, 2023 · 10 comments
Closed

[BUG] [GCC] UFCS with qualified function fails #746

JohelEGP opened this issue Oct 12, 2023 · 10 comments
Labels
bug Something isn't working

Comments

@JohelEGP
Copy link
Contributor

Title: [GCC] UFCS with qualified function fails.

Description:

Clang and MSVC accept: https://compiler-explorer.com/z/qb19TEGv1.
I haven't reported this before because I was sure that
UFCS is for member call syntax and a qualified function doesn't make sense there.
But Herb has used such calls before, and it might be actually useful: #741 (comment).

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

ns: namespace = {
  f: (_) = { }
}
main: () = {
  _ = 0.ns::f();
}
Commands:
cppfront main.cpp2
g++14 -std=c++23 -pedantic-errors -Wall -Wextra -Wconversion -Werror=unused-result -I . main.cpp

Expected result:

  • A diagnostic by Cppfront, or
  • GCC to work (maybe it's their bug), or
  • if this is a feature, for a qualified function in a member function call syntax
    to lower without the UFCS macro as a free function call because a member call would never work anyways.

Actual result and error:

Cpp2 lowered to Cpp1:
//=== Cpp2 type declarations ====================================================


#include "cpp2util.h"

namespace ns {

}


//=== Cpp2 type definitions and function declarations ===========================

namespace ns {
  auto f([[maybe_unused]] auto const& param1) -> void;
}
auto main() -> int;
  

//=== Cpp2 function definitions =================================================

namespace ns {
  auto f([[maybe_unused]] auto const& param1) -> void{}
}
auto main() -> int{
  static_cast<void>(CPP2_UFCS_0(ns::f, 0));
}
Output:
build/main.cpp: In lambda function:
build/main.cpp:27:33: error: 'ns::f' is not a class member
   27 |   static_cast<void>(CPP2_UFCS_0(ns::f, 0));
      |                                 ^~
raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:761:47: note: in definition of macro 'CPP2_UFCS_0'
  761 |     if constexpr (requires{ CPP2_FORWARD(obj).FUNCNAME(); }) { \
      |                                               ^~~~~~~~
build/main.cpp:27:33: error: 'ns::f' is not a class member
   27 |   static_cast<void>(CPP2_UFCS_0(ns::f, 0));
      |                                 ^~
raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:762:34: note: in definition of macro 'CPP2_UFCS_0'
  762 |         return CPP2_FORWARD(obj).FUNCNAME(); \
      |                                  ^~~~~~~~
@JohelEGP
Copy link
Contributor Author

and it might be actually useful: #741 (comment)

Considering Herb's comment (#741 (comment)),
it could actually be a feature because

it can help reduce the pressure to add special-purpose language features like |> and reduce pressure to use special-purpose library techniques like overloading |

Those accept |> ranges::to and | ranges::to,
unlike UFCS on GCC with .ranges::to.

@JohelEGP
Copy link
Contributor Author

a qualified function doesn't make sense there

a member call would never work anyways

Turns out there's a case.
Calling the member of a base type.
https://compiler-explorer.com/z/K8zqsGzPP:

struct B {
  void f();
};
struct D : B { };
void g() {
  D d;
  d.B::f();
}

So 00ef370 is a wrong fix.

@JohelEGP

This comment was marked as outdated.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Oct 12, 2023

Calling the member of a base type.

But all reject the UFCS macro on this (https://compiler-explorer.com/z/fe3j4ex9Y, https://cpp2.godbolt.org/z/xKT6dzTjr):

namespace N {
struct B {
  void f();
};
}
struct D : N::B { };
void g() {
  D d;
  d.B::f(); // OK.
  static_cast<void>(
    [&](auto&& obj) {
      if constexpr (requires { obj.B::f(); })
        return obj.B::f();
      else
        return B::f(obj); // Error: No `B`.
    }
  );
}

@JohelEGP
Copy link
Contributor Author

I think this is IFNDR, and GCC is right.

I was wrong.
I have opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111841
after reading https://eel.is/c++draft/temp.dep and https://eel.is/c++draft/basic.lookup.qual#def:lookup_context.

@hsutter
Copy link
Owner

hsutter commented Oct 20, 2023

Thanks for the research. So, my understanding is that this a GCC conformance bug, correct?

@JohelEGP
Copy link
Contributor Author

That's right.

@hsutter
Copy link
Owner

hsutter commented Oct 20, 2023

Thanks. OK to close?

@JohelEGP
Copy link
Contributor Author

Yes.

@JohelEGP
Copy link
Contributor Author

The implicit std::move of the function in a UFCS call triggers this bug (https://cpp2.godbolt.org/z/7WqhMYY7h):

main: () = {
  f := :(_) = { };
  _ = 0.f();
}
  static_cast<void>(CPP2_UFCS(std::move(f))(0));
main.cpp2:3:31: error: 'std::move' is not a class member

JohelEGP added a commit to JohelEGP/cppfront that referenced this issue Dec 21, 2023
Commit 4eef0da
actually worked around hsutter#746.
hsutter added a commit that referenced this issue Feb 4, 2024
* fix(sema): recognize last use immediately after declaration

* fix(sema): give function expression its own scope level

* fix(sema): consider use as function in UFCS call

* refactor(sema): remove stale comment

* test(sema): also add unit test for `move` parameter

* refactor(sema): avoid possible decrement beyond `begin`

* fix(sema): move `this` on last use

* test: update GCC 13 results

* test: add Clang 18 results

* test: add overloads with `that`

* test: add another case that doesn't work yet

* test: rename members to avoid shadowing

* fix(sema): move implicit `this` of member on last use

* test: remove commented cases that aren't last uses

* fix(sema): guard check for `move this` parameter once

* test: add case for destructor

* test: add missing GCC 13 result

* test: regenerate tests

* fix(sema): consider use as function in UFCS call

* fix(to_cpp1): type-scope variable initialization is not deferred

* refactor(util): add UFCS macros to move and forward

* fix(to_cpp1): emit last use of function in UFCS as part of macro name

* test: regenerate GCC 13 result

* fix(to_cpp1): exercise last use in range of for loop

* refactor(util): remove UFCS macros of invalid combinations

* refactor(to_cpp1): handle range of for loop specifically

* refactor(to_cpp1): consider the expression list range

* fix(sema): apply last use to call of member function

* fix(sema): do not move a member function

* fix(to_cpp1): do not move any member function

* test: remove non-problematic comment about double move

It just works, see <https://cpp1.godbolt.org/z/6fhjxrz65>.

* refactor: regenerate `reflect.h`

* refactor(sema): avoid decrementing before begin

* fix(sema): extend implicit else branch to containing statement

* fix(sema): increase scope to match an explicit else

* refactor(sema): move heuristic to not change ending depths

* fix(sema): pop consecutive implicit else branches

* refactor(parse): put implicit else statements in the actual else node

* test: add test cases that combine implicit and explicit access

* test: add unit test for all test cases as selections

* fix(reflect): fix initialization and regenerate

* fix(sema): apply sema phase to generated code

* test: regenerate results

* test: add test case for another limitation

* test: add another test case that fails

* fix(sema): improve heuristic to only move from last (implicit) `this`

* refactor(reflect): restore implicit `this` access

* fix(sema): do not stack an implicit else branch

* refactor: use "implicit scope" throughout

* fix(sema): use local `move this` to determine last use

* fix(sema): adjust condition for looking up to type

* fix(sema): use last move on called type-scope variable

* test: remove bogus comment

* fix(sema): correct unset `this.` case

* test: reference open issue

* test: add test cases not affected by the open issue

* test: clarify FIXME comment

* test: review the new tests

* perf: replace `std::map` with `mutable` data member

* refactor: add `symbol::get_final_position`

* refactor(to_cpp1): do not move single named return

* test: add new test cases to commented out test case

* test: add similar test case to callable case

* refactor(sema): rework some outdated whitespace

* test: regenerate results after "do not move single named return"

* feat(sema): diagnose unused variables in generate code

* fix(sema): consider variables in type scope declared later

* test: refactor new test to use `f_copy`

* refactor(sema): use member token

* refactor(sema): update comments

* refactor(sema): use the `this` parameter exclusively

* refactor(sema): update the comments

* refactor(sema): finish focusing on the implicit 'this'

* fix(to_cpp1): move returned uninitialized local

* fix(to_cpp1): move single named uninitialized return

* test: add case with member declared last

* refactor(sema): set condition for "at `this.`" correctly

* fix(sema): use the better lookup algorithm for this case

* fix(to_cpp1): stack current names only in namespaces

* fix(to_cpp1): stack current names of bases

* test: exercise forward in generated code

* test: add stand-in for `std::move_only_function`

* test: remove bogus test case

* refactor(to_cpp1): rename to `is_class_member_access`

* test: add test case showing limitations with base members

* test: actually exercise forward in generated code

* refactor(to_cpp1): reorder branch

* refactor(to_cpp1): remove outdated condition

* refactor(to_cpp1): rename to `is_class_member_access`

* fix: revert using the empty implicit else branch

Refs: e9cc033, 7f4a60f

* fix(sema): change algorithm to find last uses

* test: add test case for recursion logic

* refactor(sema): simplify condition for UFCS on member

* test: use `f_inout` and `f_copy` in earlier test cases

* test: enable commented out tests

* test: extend limitation unit test with alternative

* test: remove redundant explicit discards

* test: add more test cases for the new last use algorithm

* test: add missing `new<int>()`

* fix: regenerate `reflect.h`

* test: add test cases of discovered problems

* fix(sema): pop sibling branch conditions on found last use

* refactor(sema): localize variables

* fix(sema): recognize uses in iteration statements

* fix(sema): start from the last symbol, not past-the-end

* refactor(sema): add local type `pos_range`

* fix(sema): handle loops and (non) captures

* test: add similar case but without declaration

* test: regenerate results

* fix(reflect): update and regenerate `reflect.h`

* fix(sema): start for loop at its body

* refactor(sema): use `std::span`

* refactor(sema): register name deactivation at visit

* fix(sema): do not apply use to a hiding name

* fix(sema): skip hiding loop parameter

* test: revert `gcc-version.output`

* fix(sema): recognize use in _next-clause_

* test: add corner case

* fix(sema): recognize Cpp1 `using` declaration

* refactor(sema): avoid adding duplicate symbols

* refactor(sema): clarify similar members with comments

* refactor(sema): turn comment into code

* refactor(sema): modify local convenience variable

* refactor(sema): remove expression scope

* refactor(sema): use the right predicate

* refactor(sema): remove inactive, stale assertions

* refactor(sema): keep using a sentinel

* fix(sema): handle a nested true branch

* refactor(sema): revert whitespace change

* refactor(sema): fix `started_postfix_expression` simpler

* refactor(sema): revert stale fix of `scope_depth`

* refactor(sema): comment the need of `final_position` at hand-out

* refactor(to_cpp1): drop periods from comment

* refactor(to_cpp1): reorder arguments for better formatting

* refactor(to_cpp1): remove stale non-rvalue context

* refactor(to_cpp1): remove useless non-rvalue context

* refactor(to_cpp1): clarifiy comment with example

* test: regenerate gcc-13 results

Commit 4eef0da
actually worked around #746.

* test: regenerate results

* refactor(sema): resolve CI issues

* test: revert changes to gcc-13 result

* refactor: generalize to `identifier_sym::safe_to_move`

* test: fix implementation of `identity`

* refactor(sema): add scoped indices of uses

* refactor(sema): simplify names of activation entities

* fix(sema): do not mark non-captures as captures

* refactor(sema): rename to avoid verb prefix

According to <#231 (comment)>:
> to avoid dealing with English verb-to-adjective conventions

* fix(sema): disable implicit move unsequenced with another use

* fix(sema): consider _is-as-expression_ too

* fix(sema): check all parameters for use

* refactor(sema): remove wrong fix for UFCS issue

* Minor updates to compile cppfront and the new test cleanly using MSVC

And re-remove a few stray `;` that were removed as part of PR #911 and now cause errors because of the pedantic builds

I regenerated `reflect.h` and found two changes that weren't already in the PR, so committing those too

Also including the new test file's run against MSVC showing the five messages mentioned in the PR review, see PR review for more...

* refactor(to_cpp1): pass `0` to `int` parameter instead of `false`

* test: discard unused results

* refactor(to_cpp1): avoid the UFCS macro to workaround compiler bugs

* test: update GCC 13 results

* test: remove Clang 18 results

* refactor(sema): avoid pointer arithmetic

* test: regenerate results

* refactor(sema): avoid pointer arithmetic

* Add regression test result diffs from my machine

MSVC error is resolved

New Clang 12 error in `pure2-last-use.cpp2:938`

* test: comment Clang 12 limitation

* test: apply CI patches

* test: apply CI patches

* test: apply CI patches

* Tweak: Change "final position" to "global token order"

* Minor tweaks

While I'm at it, clean up redundant headers that now we get from cpp2util.h
And it's updating those regression test line-ends...

---------

Signed-off-by: Johel Ernesto Guerrero Peña <johelegp@gmail.com>
Signed-off-by: Herb Sutter <herb.sutter@gmail.com>
Co-authored-by: Herb Sutter <herb.sutter@gmail.com>
bluetarpmedia pushed a commit to bluetarpmedia/cppfront that referenced this issue Feb 7, 2024
* fix(sema): recognize last use immediately after declaration

* fix(sema): give function expression its own scope level

* fix(sema): consider use as function in UFCS call

* refactor(sema): remove stale comment

* test(sema): also add unit test for `move` parameter

* refactor(sema): avoid possible decrement beyond `begin`

* fix(sema): move `this` on last use

* test: update GCC 13 results

* test: add Clang 18 results

* test: add overloads with `that`

* test: add another case that doesn't work yet

* test: rename members to avoid shadowing

* fix(sema): move implicit `this` of member on last use

* test: remove commented cases that aren't last uses

* fix(sema): guard check for `move this` parameter once

* test: add case for destructor

* test: add missing GCC 13 result

* test: regenerate tests

* fix(sema): consider use as function in UFCS call

* fix(to_cpp1): type-scope variable initialization is not deferred

* refactor(util): add UFCS macros to move and forward

* fix(to_cpp1): emit last use of function in UFCS as part of macro name

* test: regenerate GCC 13 result

* fix(to_cpp1): exercise last use in range of for loop

* refactor(util): remove UFCS macros of invalid combinations

* refactor(to_cpp1): handle range of for loop specifically

* refactor(to_cpp1): consider the expression list range

* fix(sema): apply last use to call of member function

* fix(sema): do not move a member function

* fix(to_cpp1): do not move any member function

* test: remove non-problematic comment about double move

It just works, see <https://cpp1.godbolt.org/z/6fhjxrz65>.

* refactor: regenerate `reflect.h`

* refactor(sema): avoid decrementing before begin

* fix(sema): extend implicit else branch to containing statement

* fix(sema): increase scope to match an explicit else

* refactor(sema): move heuristic to not change ending depths

* fix(sema): pop consecutive implicit else branches

* refactor(parse): put implicit else statements in the actual else node

* test: add test cases that combine implicit and explicit access

* test: add unit test for all test cases as selections

* fix(reflect): fix initialization and regenerate

* fix(sema): apply sema phase to generated code

* test: regenerate results

* test: add test case for another limitation

* test: add another test case that fails

* fix(sema): improve heuristic to only move from last (implicit) `this`

* refactor(reflect): restore implicit `this` access

* fix(sema): do not stack an implicit else branch

* refactor: use "implicit scope" throughout

* fix(sema): use local `move this` to determine last use

* fix(sema): adjust condition for looking up to type

* fix(sema): use last move on called type-scope variable

* test: remove bogus comment

* fix(sema): correct unset `this.` case

* test: reference open issue

* test: add test cases not affected by the open issue

* test: clarify FIXME comment

* test: review the new tests

* perf: replace `std::map` with `mutable` data member

* refactor: add `symbol::get_final_position`

* refactor(to_cpp1): do not move single named return

* test: add new test cases to commented out test case

* test: add similar test case to callable case

* refactor(sema): rework some outdated whitespace

* test: regenerate results after "do not move single named return"

* feat(sema): diagnose unused variables in generate code

* fix(sema): consider variables in type scope declared later

* test: refactor new test to use `f_copy`

* refactor(sema): use member token

* refactor(sema): update comments

* refactor(sema): use the `this` parameter exclusively

* refactor(sema): update the comments

* refactor(sema): finish focusing on the implicit 'this'

* fix(to_cpp1): move returned uninitialized local

* fix(to_cpp1): move single named uninitialized return

* test: add case with member declared last

* refactor(sema): set condition for "at `this.`" correctly

* fix(sema): use the better lookup algorithm for this case

* fix(to_cpp1): stack current names only in namespaces

* fix(to_cpp1): stack current names of bases

* test: exercise forward in generated code

* test: add stand-in for `std::move_only_function`

* test: remove bogus test case

* refactor(to_cpp1): rename to `is_class_member_access`

* test: add test case showing limitations with base members

* test: actually exercise forward in generated code

* refactor(to_cpp1): reorder branch

* refactor(to_cpp1): remove outdated condition

* refactor(to_cpp1): rename to `is_class_member_access`

* fix: revert using the empty implicit else branch

Refs: e9cc033, 7f4a60f

* fix(sema): change algorithm to find last uses

* test: add test case for recursion logic

* refactor(sema): simplify condition for UFCS on member

* test: use `f_inout` and `f_copy` in earlier test cases

* test: enable commented out tests

* test: extend limitation unit test with alternative

* test: remove redundant explicit discards

* test: add more test cases for the new last use algorithm

* test: add missing `new<int>()`

* fix: regenerate `reflect.h`

* test: add test cases of discovered problems

* fix(sema): pop sibling branch conditions on found last use

* refactor(sema): localize variables

* fix(sema): recognize uses in iteration statements

* fix(sema): start from the last symbol, not past-the-end

* refactor(sema): add local type `pos_range`

* fix(sema): handle loops and (non) captures

* test: add similar case but without declaration

* test: regenerate results

* fix(reflect): update and regenerate `reflect.h`

* fix(sema): start for loop at its body

* refactor(sema): use `std::span`

* refactor(sema): register name deactivation at visit

* fix(sema): do not apply use to a hiding name

* fix(sema): skip hiding loop parameter

* test: revert `gcc-version.output`

* fix(sema): recognize use in _next-clause_

* test: add corner case

* fix(sema): recognize Cpp1 `using` declaration

* refactor(sema): avoid adding duplicate symbols

* refactor(sema): clarify similar members with comments

* refactor(sema): turn comment into code

* refactor(sema): modify local convenience variable

* refactor(sema): remove expression scope

* refactor(sema): use the right predicate

* refactor(sema): remove inactive, stale assertions

* refactor(sema): keep using a sentinel

* fix(sema): handle a nested true branch

* refactor(sema): revert whitespace change

* refactor(sema): fix `started_postfix_expression` simpler

* refactor(sema): revert stale fix of `scope_depth`

* refactor(sema): comment the need of `final_position` at hand-out

* refactor(to_cpp1): drop periods from comment

* refactor(to_cpp1): reorder arguments for better formatting

* refactor(to_cpp1): remove stale non-rvalue context

* refactor(to_cpp1): remove useless non-rvalue context

* refactor(to_cpp1): clarifiy comment with example

* test: regenerate gcc-13 results

Commit 4eef0da
actually worked around hsutter#746.

* test: regenerate results

* refactor(sema): resolve CI issues

* test: revert changes to gcc-13 result

* refactor: generalize to `identifier_sym::safe_to_move`

* test: fix implementation of `identity`

* refactor(sema): add scoped indices of uses

* refactor(sema): simplify names of activation entities

* fix(sema): do not mark non-captures as captures

* refactor(sema): rename to avoid verb prefix

According to <hsutter#231 (comment)>:
> to avoid dealing with English verb-to-adjective conventions

* fix(sema): disable implicit move unsequenced with another use

* fix(sema): consider _is-as-expression_ too

* fix(sema): check all parameters for use

* refactor(sema): remove wrong fix for UFCS issue

* Minor updates to compile cppfront and the new test cleanly using MSVC

And re-remove a few stray `;` that were removed as part of PR hsutter#911 and now cause errors because of the pedantic builds

I regenerated `reflect.h` and found two changes that weren't already in the PR, so committing those too

Also including the new test file's run against MSVC showing the five messages mentioned in the PR review, see PR review for more...

* refactor(to_cpp1): pass `0` to `int` parameter instead of `false`

* test: discard unused results

* refactor(to_cpp1): avoid the UFCS macro to workaround compiler bugs

* test: update GCC 13 results

* test: remove Clang 18 results

* refactor(sema): avoid pointer arithmetic

* test: regenerate results

* refactor(sema): avoid pointer arithmetic

* Add regression test result diffs from my machine

MSVC error is resolved

New Clang 12 error in `pure2-last-use.cpp2:938`

* test: comment Clang 12 limitation

* test: apply CI patches

* test: apply CI patches

* test: apply CI patches

* Tweak: Change "final position" to "global token order"

* Minor tweaks

While I'm at it, clean up redundant headers that now we get from cpp2util.h
And it's updating those regression test line-ends...

---------

Signed-off-by: Johel Ernesto Guerrero Peña <johelegp@gmail.com>
Signed-off-by: Herb Sutter <herb.sutter@gmail.com>
Co-authored-by: Herb Sutter <herb.sutter@gmail.com>
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
Development

No branches or pull requests

2 participants