Reject duplicate class member names involving properties

The LRM requires an identifier to name only one item within a scope. Class
properties are currently stored only in `class_type_t::properties`, outside
the common local symbol table. As a result, declarations such as:

    class C;
      int value;
      function int value;
        return 0;
      endfunction
    endclass

are accepted. A second property in a separate declaration silently replaces
the first property map entry. A repeated name in a comma-separated declaration
such as `int value, value` can crash because both declarators share the same raw
type.

Make `prop_info_t` a named item and register each property in the class local
symbol table. Check the declaration before taking ownership of its type or
initializer, since comma-separated properties share the raw declaration type.
Use `emplace()` for the property map so an accepted property cannot replace an
existing entry.

Properties now take part in the common symbol lookup. Give them a distinct
symbol type so duplicate declaration diagnostics identify them as class
properties, and remove the separate property check from type-identifier
classification.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

PNamedItem.cc

@@ -48,6 +48,9 @@ std::ostream& operator << (std::ostream&o, PNamedItem::SymbolType st)
           case PNamedItem::VAR:
             o << "a variable";
             break;
+          case PNamedItem::CLASS_PROPERTY:
+            o << "a class property";
+            break;
           case PNamedItem::GENVAR:
             o << "a genvar";
             break;

PNamedItem.h

@@ -31,7 +31,7 @@ class PNamedItem : virtual public LineInfo {
       enum SymbolType { ANY, PARAM, NET, VAR, GENVAR, EVENT, TYPE, ENUM,
                         CLASS, FUNCTION, TASK, BLOCK, GENBLOCK, MODPORT,
                         PACKAGE, MODULE, PROGRAM, INTERFACE, PRIMITIVE,
-                        INSTANCE };
+                        INSTANCE, CLASS_PROPERTY };
 
       explicit PNamedItem();
       virtual ~PNamedItem() override;

pform.cc

@@ -453,7 +453,8 @@ static PGenerate* current_generate_scope()
       return lexical_scope == pform_cur_generate ? pform_cur_generate : nullptr;
 }
 
-static void add_local_symbol(LexicalScope*scope, perm_string name, PNamedItem*item)
+bool pform_check_local_symbol(LexicalScope *scope, perm_string name,
+			      const PNamedItem *item)
 {
       assert(scope);
 
@@ -468,7 +469,7 @@ static void add_local_symbol(LexicalScope*scope, perm_string name, PNamedItem*it
 		    "It was declared here as "
 		 << cur_sym->second->symbol_type() << "." << endl;
 	    error_count += 1;
-	    return;
+	    return false;
       }
 
 	// Check for conflict with an explicit import.
@@ -479,9 +480,17 @@ static void add_local_symbol(LexicalScope*scope, perm_string name, PNamedItem*it
 		    "imported into this scope from package '"
 		 << cur_pkg->second.package->pscope_name() << "'." << endl;
 	    error_count += 1;
-	    return;
+	    return false;
       }
 
+      return true;
+}
+
+static void add_local_symbol(LexicalScope*scope, perm_string name, PNamedItem*item)
+{
+      if (!pform_check_local_symbol(scope, name, item))
+	    return;
+
       scope->local_symbols[name] = item;
 }
 
@@ -951,16 +960,6 @@ typedef_t* pform_test_type_identifier(const struct vlltype&loc, const char*txt)
 	    if (sym != cur_scope->local_symbols.end())
 		  return nullptr;
 
-	    // Class properties are tracked in the class type, not in
-	    // local_symbols, but still shadow type names before lookup falls
-	    // through to wildcard imports.
-	    if (auto cur_class = dynamic_cast<PClass*> (cur_scope)) {
-		  if (cur_class->type &&
-		      cur_class->type->properties.find(name) !=
-		      cur_class->type->properties.end())
-			return nullptr;
-	    }
-
             PPackage*pkg = pform_find_potential_import(loc, cur_scope, name, false, false);
             if (pkg) {
 	          cur = pkg->typedefs.find(name);

pform.h

@@ -259,6 +259,9 @@ extern void pform_pop_block_scope(bool keep_scope);
  */
 extern LexicalScope* pform_peek_scope();
 
+extern bool pform_check_local_symbol(LexicalScope *scope, perm_string name,
+				     const PNamedItem *item);
+
 extern PClass* pform_push_class_scope(const struct vlltype&loc, perm_string name);
 
 extern PFunction*pform_push_constructor_scope(const struct vlltype&loc);

pform_pclass.cc

@@ -18,6 +18,7 @@
  */
 
 # include  <cstdarg>
+# include  <utility>
 # include  "pform.h"
 # include  "PClass.h"
 # include  "parse_misc.h"
@@ -78,6 +79,30 @@ void pform_class_property(const struct vlltype&loc,
 		 ; cur != decls->end() ; ++cur) {
 
 	    decl_assignment_t*curp = *cur;
+	    class_type_t::prop_info_t property(property_qual, nullptr,
+						 curp->expr != nullptr);
+	    FILE_NAME(&property, loc);
+
+	    // Properties are stored by value in the property map, while the type
+	    // can be shared by comma-separated declarations. Check the temporary
+	    // before taking ownership of the type or releasing the initializer. It
+	    // can not be registered in the local symbol table yet since that would
+	    // store a pointer to the temporary.
+	    if (!pform_check_local_symbol(pform_cur_class, curp->name.first,
+					  &property))
+		  continue;
+
+	    auto result = pform_cur_class->type->properties.emplace(
+		  curp->name.first, std::move(property));
+	    ivl_assert(loc, result.second);
+
+	    auto &property_info = result.first->second;
+	    // Register the property now that it has a stable address. The earlier
+	    // check makes failure of this insertion an internal error.
+	    auto symbol = pform_cur_class->local_symbols.emplace(
+		  curp->name.first, &property_info);
+	    ivl_assert(loc, symbol.second);
+
 	    data_type_t*use_type = data_type;
 
 	    if (! curp->index.empty()) {
@@ -86,10 +111,7 @@ void pform_class_property(const struct vlltype&loc,
 		  FILE_NAME(use_type, loc);
 	    }
 
-	    pform_cur_class->type->properties[curp->name.first]
-		  = class_type_t::prop_info_t(property_qual, use_type,
-					    curp->expr != nullptr);
-	    FILE_NAME(&pform_cur_class->type->properties[curp->name.first], loc);
+	    property_info.type.reset(use_type);
 
 	    if (PExpr*rval = curp->expr.release()) {
 		  PExpr*lval = new PEIdent(curp->name.first, curp->name.second);

pform_types.h

@@ -408,13 +408,14 @@ struct class_type_t : public data_type_t {
       bool virtual_class;
 
 	// This is a map of the properties. Map the name to the type.
-      struct prop_info_t : public LineInfo {
+      struct prop_info_t : public PNamedItem {
 	    inline prop_info_t() : qual(property_qualifier_t::make_none()) { }
 	    inline prop_info_t(property_qualifier_t q, data_type_t *t,
 			       bool init)
 	    : qual(q), type(t), has_initializer(init) { }
 	    prop_info_t(prop_info_t&&) = default;
 	    prop_info_t& operator=(prop_info_t&&) = default;
+	    SymbolType symbol_type() const override { return CLASS_PROPERTY; }
 	    property_qualifier_t qual;
 	    std::unique_ptr<data_type_t> type;
 	    bool has_initializer = false;