[Patch, Fortran] PR 85599: warn about short-circuiting of logical expressions for non-pure functions

[Janus Weil <janus@gcc.gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]

Hi all,

after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions.

As a reminder (and recap of the previous discussion), the Fortran
standard unfortunately is a bit sloppy in this area: It allows
compilers to short-circuit the second operand of .AND. / .OR.
operators, but does not require this. As a result, compilers can do
what they want without conflicting with the standard, and they do:
gfortran does short-circuiting (via TRUTH_ANDIF_EXPR/TRUTH_ORIF_EXPR),
ifort does not.

I'm continuing here the least-invasive approach of keeping gfortran's
current behavior, but warning about cases where compilers may produce
different results.

The attached patch is very close to the version I posted previously
(which was already approved by Janne), with the difference that the
warnings are now triggered by -Wextra and not -Wsurprising (which is
included in -Wall), as suggested by Nick Maclaren. I think this is
more reasonable, since not everyone may want to see these warnings.

Note that I don't want to warn about all possible optimizations that
might be allowed by the standard, but only about those that are
actually problematic in practice and result in compiler-dependent
behavior.

The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk?

Cheers,
Janus


2018-07-11  Thomas Koenig  <tkoenig@gcc.gnu.org>
        Janus Weil  <janus@gcc.gnu.org>

        PR fortran/85599
        * dump-parse-tree (show_attr): Add handling of implicit_pure.
        * resolve.c (impure_function_callback): New function.
        (resolve_operator): Call it vial gfc_expr_walker.


2018-07-11  Janus Weil  <janus@gcc.gnu.org>

        PR fortran/85599
        * gfortran.dg/short_circuiting.f90: New test.

[-- Attachment #2: pr85599_v2.diff --]
[-- Type: text/x-patch, Size: 2415 bytes --]

Index: gcc/fortran/dump-parse-tree.c
===================================================================
--- gcc/fortran/dump-parse-tree.c	(revision 262563)
+++ gcc/fortran/dump-parse-tree.c	(working copy)
@@ -716,6 +716,8 @@ show_attr (symbol_attribute *attr, const char * mo
     fputs (" ELEMENTAL", dumpfile);
   if (attr->pure)
     fputs (" PURE", dumpfile);
+  if (attr->implicit_pure)
+    fputs (" IMPLICIT_PURE", dumpfile);
   if (attr->recursive)
     fputs (" RECURSIVE", dumpfile);
 
Index: gcc/fortran/resolve.c
===================================================================
--- gcc/fortran/resolve.c	(revision 262563)
+++ gcc/fortran/resolve.c	(working copy)
@@ -3822,6 +3822,46 @@ lookup_uop_fuzzy (const char *op, gfc_symtree *uop
 }
 
 
+/* Callback finding an impure function as an operand to an .and. or
+   .or.  expression.  Remember the last function warned about to
+   avoid double warnings when recursing.  */
+
+static int
+impure_function_callback (gfc_expr **e, int *walk_subtrees ATTRIBUTE_UNUSED,
+			  void *data)
+{
+  gfc_expr *f = *e;
+  const char *name;
+  static gfc_expr *last = NULL;
+  bool *found = (bool *) data;
+
+  if (f->expr_type == EXPR_FUNCTION)
+    {
+      *found = 1;
+      if (f != last && !pure_function (f, &name))
+	{
+	  /* This could still be a function without side effects, i.e.
+	     implicit pure.  Do not warn for that case.  */
+	  if (f->symtree == NULL || f->symtree->n.sym == NULL
+	      || !gfc_implicit_pure (f->symtree->n.sym))
+	    {
+	      if (name)
+		gfc_warning (OPT_Wextra,
+			     "Function %qs at %L might not be evaluated",
+			     name, &f->where);
+	      else
+		gfc_warning (OPT_Wextra,
+			     "Function at %L might not be evaluated",
+			     &f->where);
+	    }
+	}
+      last = f;
+    }
+
+  return 0;
+}
+
+
 /* Resolve an operator expression node.  This can involve replacing the
    operation with a user defined function call.  */
 
@@ -3930,6 +3970,14 @@ resolve_operator (gfc_expr *e)
 	    gfc_convert_type (op1, &e->ts, 2);
 	  else if (op2->ts.kind < e->ts.kind)
 	    gfc_convert_type (op2, &e->ts, 2);
+
+	  if (e->value.op.op == INTRINSIC_AND || e->value.op.op == INTRINSIC_OR)
+	    {
+	      /* Warn about short-circuiting
+	         with impure function as second operand.  */
+	      bool op2_f = false;
+	      gfc_expr_walker (&op2, impure_function_callback, &op2_f);
+	    }
 	  break;
 	}
 

[-- Attachment #3: short_circuiting.f90 --]
[-- Type: text/x-fortran, Size: 629 bytes --]

! { dg-do compile }
! { dg-additional-options "-Wextra" }
!
! PR 85599: warn about short-circuiting of logical expressions for non-pure functions
!
! Contributed by Janus Weil <janus@gcc.gnu.org>

program short_circuit

   logical :: flag
   flag = .false.
   flag = check() .and. flag
   flag = flag .and. check()      ! { dg-warning "might not be evaluated" }
   flag = flag .and. pure_check()

contains

   logical function check()
      integer, save :: i = 1
      print *, "check", i
      i = i + 1
      check = .true.
   end function

   logical pure function pure_check()
      pure_check = .true.
   end function

end