Fix ERC global label unit tests

Need to test all units in the subgraph as there are chances that the
subgraph might have more than one label, which needs to be consistently
handled

eeschema/connection_graph.cpp

@@ -2953,13 +2953,11 @@ bool CONNECTION_GRAPH::ercCheckFloatingWires( const CONNECTION_SUBGRAPH* aSubgra
 bool CONNECTION_GRAPH::ercCheckLabels( const CONNECTION_SUBGRAPH* aSubgraph )
 {
     // Label connection rules:
+    // Any label without a no-connect needs to have at least 2 pins, otherwise it is invalid
     // Local labels are flagged if they don't connect to any pins and don't have a no-connect
     // Global labels are flagged if they appear only once, don't connect to any local labels,
     // and don't have a no-connect marker
 
-    // So, if there is a no-connect, we will never generate a warning here
-    if( aSubgraph->m_no_connect )
-        return true;
 
     if( !aSubgraph->m_driver_connection )
         return true;
@@ -2971,28 +2969,46 @@ bool CONNECTION_GRAPH::ercCheckLabels( const CONNECTION_SUBGRAPH* aSubgraph )
 
     ERC_SETTINGS& settings            = m_schematic->ErcSettings();
     bool          ok                  = true;
-    SCH_TEXT*     text                = nullptr;
-    bool          hasOtherConnections = false;
     int           pinCount            = 0;
 
+    std::map<KICAD_T, std::vector<SCH_TEXT*>> label_map;
+
+
     auto hasPins =
-            []( const CONNECTION_SUBGRAPH* aLocSubgraph )
+            []( const CONNECTION_SUBGRAPH* aLocSubgraph ) -> int
             {
+                int retval = 0;
+
                 for( const SCH_ITEM* item : aLocSubgraph->m_items )
                 {
                     switch( item->Type() )
                     {
                     case SCH_PIN_T:
                     case SCH_SHEET_PIN_T:
-                        return true;
+                        ++retval;
+                        break;
 
                     default: break;
                     }
                 }
 
-                return false;
+                return retval;
             };
 
+    auto reportError = [&]( SCH_TEXT* aText, int errCode )
+    {
+        if( settings.IsTestEnabled( errCode ) )
+        {
+            std::shared_ptr<ERC_ITEM> ercItem = ERC_ITEM::Create( errCode );
+            ercItem->SetItems( aText );
+
+            SCH_MARKER* marker = new SCH_MARKER( ercItem, aText->GetPosition() );
+            aSubgraph->m_sheet.LastScreen()->Append( marker );
+        }
+    };
+
+    pinCount = hasPins( aSubgraph );
+
     for( SCH_ITEM* item : aSubgraph->m_items )
     {
         switch( item->Type() )
@@ -3001,105 +3017,134 @@ bool CONNECTION_GRAPH::ercCheckLabels( const CONNECTION_SUBGRAPH* aSubgraph )
         case SCH_GLOBAL_LABEL_T:
         case SCH_HIER_LABEL_T:
         {
-            text = static_cast<SCH_TEXT*>( item );
+            SCH_TEXT* text = static_cast<SCH_TEXT*>( item );
+
+            label_map[item->Type()].push_back( text );
 
             // Below, we'll create an ERC if the whole subgraph is unconnected.  But, additionally,
             // we want to error if an individual label in the subgraph is floating, even if it's
             // connected to other valid things by way of another label on the same sheet.
-
+            if( text->IsDangling() )
-            if( text->IsDangling() && settings.IsTestEnabled( ERCE_LABEL_NOT_CONNECTED ) )
             {
-                std::shared_ptr<ERC_ITEM> ercItem = ERC_ITEM::Create( ERCE_LABEL_NOT_CONNECTED );
+                reportError( text, ERCE_LABEL_NOT_CONNECTED );
-                ercItem->SetItems( text );
+                return false;
-
-                SCH_MARKER* marker = new SCH_MARKER( ercItem, text->GetPosition() );
-                aSubgraph->m_sheet.LastScreen()->Append( marker );
-                ok = false;
             }
 
             break;
         }
 
-        case SCH_PIN_T:
+        // If this subgraph has a no-connect, do not continue processing as we do not
-        case SCH_SHEET_PIN_T:
+        // submit no-connect errors for nets explicitly designated as no-connect
-            pinCount++;
+        case SCH_NO_CONNECT_T:
-            break;
+            return true;
 
         default:
             break;
         }
     }
 
-    if( !text )
+    if( label_map.empty() )
         return true;
 
-    bool isGlobal = text->Type() == SCH_GLOBAL_LABEL_T;
-    int  errCode = isGlobal ? ERCE_GLOBLABEL : ERCE_LABEL_NOT_CONNECTED;
-
     wxCHECK_MSG( m_schematic, true, "Null m_schematic in CONNECTION_GRAPH::ercCheckLabels" );
 
-    wxString name = EscapeString( text->GetShownText(), CTX_NETNAME );
-
     // Labels that have multiple pins connected are not dangling (may be used for naming segments)
     // so leave them without errors here
     if( pinCount > 1 )
         return true;
 
-    if( isGlobal )
+    for( auto& [type, label_vec] : label_map )
     {
-        // Global labels are connected if there is another instance of their name in the circuit
+        switch( type )
-        auto it = m_net_name_to_subgraphs_map.find( name );
+        {
+        case SCH_GLOBAL_LABEL_T:
+            if( !settings.IsTestEnabled( ERCE_GLOBLABEL ) )
+                break;
 
-        if( it != m_net_name_to_subgraphs_map.end() )
+            for( SCH_TEXT* text : label_vec )
-        {
-            if( it->second.size() > 1 || aSubgraph->m_multiple_drivers )
-                hasOtherConnections = true;
-        }
-    }
-    else if( text->Type() == SCH_HIER_LABEL_T )
-    {
-        // Hierarchical labels are connected if their parents are connected
-        if( aSubgraph->m_hier_parent
-                && ( aSubgraph->m_hier_parent->m_strong_driver
-                        || aSubgraph->m_hier_parent->m_drivers.size() > 1) )
-        {
-            // For now, a simple check: if there is more than one driver, the parent is probably
-            // connected elsewhere (because at least one driver will be the hier pin itself)
-            hasOtherConnections = true;
-        }
-    }
-    else
-    {
-        auto pair = std::make_pair( aSubgraph->m_sheet, name );
-        auto it = m_local_label_cache.find( pair );
-
-        if( it != m_local_label_cache.end() && it->second.size() > 1 )
-        {
-            for( const CONNECTION_SUBGRAPH* neighbor : it->second )
             {
-                if( neighbor == aSubgraph )
+                wxString name = EscapeString( text->GetShownText(), CTX_NETNAME );
+                int allPins = pinCount;
+
+                // If the global label is connected to a subgraph but is not the driver
+                // then there is a driver conflict somewhere that is handled by a different
+                // ERC check.  Here, we are just looking for unconnected elements
+                if( name != aSubgraph->m_driver_connection->Name() )
                     continue;
 
-                if( hasPins( neighbor ) && ++pinCount > 1 )
+                // Global labels are connected if there is another instance of
+                // their name in the circuit
+                auto it = m_net_name_to_subgraphs_map.find( name );
+
+                for( const CONNECTION_SUBGRAPH* neighbor : it->second )
                 {
-                    hasOtherConnections = true;
+                    if( neighbor == aSubgraph )
-                    break;
+                        continue;
+
+                    allPins += hasPins( neighbor );
+                }
+
+                if( allPins < 2 )
+                {
+                    reportError( text, ERCE_GLOBLABEL );
+                    ok = false;
                 }
             }
+
+            break;
+        case SCH_HIER_LABEL_T:
+            if( !settings.IsTestEnabled( ERCE_LABEL_NOT_CONNECTED ) )
+                break;
+
+            for( SCH_TEXT* text : label_vec )
+            {
+                if( !aSubgraph->m_hier_parent
+                        || ( !aSubgraph->m_hier_parent->m_strong_driver
+                                && aSubgraph->m_hier_parent->m_drivers.size() <= 1 ) )
+                {
+                    reportError( text, ERCE_LABEL_NOT_CONNECTED );
+                    ok = false;
+                }
+            }
+
+            break;
+        case SCH_LABEL_T:
+
+            if( !settings.IsTestEnabled( ERCE_LABEL_NOT_CONNECTED ) )
+                break;
+
+            for( SCH_TEXT* text : label_vec )
+            {
+                int allPins = pinCount;
+                wxString name = EscapeString( text->GetShownText(), CTX_NETNAME );
+
+                auto pair = std::make_pair( aSubgraph->m_sheet, name );
+                auto it = m_local_label_cache.find( pair );
+
+                if( it == m_local_label_cache.end() )
+                    continue;
+
+                for( const CONNECTION_SUBGRAPH* neighbor : it->second )
+                {
+                    if( neighbor == aSubgraph )
+                        continue;
+
+                    allPins += hasPins( neighbor );
+                }
+
+                if( allPins < 2 )
+                {
+                    reportError( text, ERCE_LABEL_NOT_CONNECTED );
+                    ok = false;
+                }
+            }
+
+            break;
+        default:
+            break;
         }
     }
 
-    if( !hasOtherConnections && settings.IsTestEnabled( errCode ) )
-    {
-        std::shared_ptr<ERC_ITEM> ercItem = ERC_ITEM::Create( errCode );
-        ercItem->SetItems( text );
-
-        SCH_MARKER* marker = new SCH_MARKER( ercItem, text->GetPosition() );
-        aSubgraph->m_sheet.LastScreen()->Append( marker );
-
-        return false;
-    }
-
     return ok;
 }
 

qa/data/eeschema/issue9367.kicad_sch

@@ -5,51 +5,178 @@
   (paper "A4")
 
   (lib_symbols
+    (symbol "Device:R" (pin_numbers hide) (pin_names (offset 0)) (in_bom yes) (on_board yes)
+      (property "Reference" "R" (id 0) (at 2.032 0 90)        (effects (font (size 1.27 1.27)))
+      
+)
+      (property "Value" "R" (id 1) (at 0 0 90)        (effects (font (size 1.27 1.27)))
+      
+)
+      (property "Footprint" "" (id 2) (at -1.778 0 90)        (effects (font (size 1.27 1.27)) hide)
+      
+)
+      (property "Datasheet" "~" (id 3) (at 0 0 0)        (effects (font (size 1.27 1.27)) hide)
+      
+)
+      (property "ki_keywords" "R res resistor" (id 4) (at 0 0 0)        (effects (font (size 1.27 1.27)) hide)
+      
+)
+      (property "ki_description" "Resistor" (id 5) (at 0 0 0)        (effects (font (size 1.27 1.27)) hide)
+      
+)
+      (property "ki_fp_filters" "R_*" (id 6) (at 0 0 0)        (effects (font (size 1.27 1.27)) hide)
+      
+)
+      (symbol "R_0_1"
+        (rectangle (start -1.016 -2.54) (end 1.016 2.54)
+          (stroke (width 0.254) (type default))
+          (fill (type none))
+        )
+      )
+      (symbol "R_1_1"
+        (pin passive line (at 0 3.81 270) (length 1.27)
+          (name "~" (effects (font (size 1.27 1.27))))
+          (number "1" (effects (font (size 1.27 1.27))))
+        )
+        (pin passive line (at 0 -3.81 90) (length 1.27)
+          (name "~" (effects (font (size 1.27 1.27))))
+          (number "2" (effects (font (size 1.27 1.27))))
+        )
+      )
+    )
   )
 
 
 
+  (no_connect (at 91.44 110.49) (uuid 9b79e0a6-69e3-486b-a127-79b95ef7faf4))
   (no_connect (at 121.92 78.74) (uuid de36d74a-ca9f-4049-b9b3-1ae40137e716))
 
+  (wire (pts (xy 238.76 109.22) (xy 240.03 109.22))
+    (stroke (width 0) (type default))
+    (uuid 1e1d8af3-0945-4386-935a-633c882fe7dc)
+  )
+  (wire (pts (xy 238.76 119.38) (xy 238.76 118.11))
+    (stroke (width 0) (type default))
+    (uuid 388b001d-f183-41e3-80f6-f75fd4a9783f)
+  )
   (wire (pts (xy 121.92 78.74) (xy 138.43 78.74))
     (stroke (width 0) (type default))
     (uuid 5f3fe4fd-c13a-48e9-91d7-50e9781364e3)
   )
+  (wire (pts (xy 238.76 110.49) (xy 238.76 109.22))
+    (stroke (width 0) (type default))
+    (uuid 7f2fc672-b99b-4383-aea3-805127501d7f)
+  )
+  (wire (pts (xy 87.63 124.46) (xy 91.44 124.46))
+    (stroke (width 0) (type default))
+    (uuid 8b08992f-318f-4afb-a970-d3dcd260f9c1)
+  )
   (wire (pts (xy 121.92 71.12) (xy 138.43 71.12))
     (stroke (width 0) (type default))
     (uuid b929aaf4-57da-46c1-8ebd-39696966af45)
   )
+  (wire (pts (xy 237.49 85.09) (xy 241.3 85.09))
+    (stroke (width 0) (type default))
+    (uuid ceea9090-e40c-4b56-a6c4-9affe28ee2bf)
+  )
   (wire (pts (xy 124.46 97.79) (xy 135.89 97.79))
     (stroke (width 0) (type default))
     (uuid ed2d734a-96ab-4f8d-9964-b35fb6f264e1)
   )
+  (wire (pts (xy 240.03 119.38) (xy 238.76 119.38))
+    (stroke (width 0) (type default))
+    (uuid f08b64b3-ebe7-446d-a4fd-9fd5fb79878b)
+  )
+  (wire (pts (xy 87.63 118.11) (xy 91.44 118.11))
+    (stroke (width 0) (type default))
+    (uuid f1260963-6de3-4110-b0ad-097bc1b7c86d)
+  )
+  (wire (pts (xy 237.49 92.71) (xy 241.3 92.71))
+    (stroke (width 0) (type default))
+    (uuid f8b5dc92-b396-4f6f-8c6d-0cef84d50e1f)
+  )
 
+  (text_box "We do not raise an unconnected error here because the net is explicitly designated 'no-connect'"
+    (at 86.36 73.66 0) (size 30.48 10.16)
+    (stroke (width 0) (type default))
+    (fill (type none))
+    (effects (font (size 1.27 1.27)) (justify left top))
+    (uuid 4a3fcd23-1beb-412d-a70f-04fbd4523388)
+  )
   (text_box "Global labels connect to local labels, so this should be an error connecting RX1 to TX1"
-    (at 146.05 68.58 0) (size 30.48 10.16)
+    (at 146.05 64.77 0) (size 30.48 10.16)
     (stroke (width 0) (type default))
     (fill (type none))
     (effects (font (size 1.27 1.27)) (justify left top))
     (uuid 8b7979c5-91e0-4622-b5f7-46261e54852f)
   )
-  (text_box "Because GL2 is subsumed, it should raise an error about not being connected elsewhere"
+  (text_box "LL2 labels are unconnected because they only have 1 pin"
-    (at 86.36 95.25 0) (size 30.48 8.89)
+    (at 69.85 110.49 0) (size 13.97 15.24)
+    (stroke (width 0) (type default))
+    (fill (type none))
+    (effects (font (size 1.27 1.27)) (justify right top))
+    (uuid 9789e6e6-671b-4e8c-a6db-ccfd56ecbf34)
+  )
+  (text_box "An error is raised here that `GL1` is not connected anywhere else in the schematic (i.e. there are no pins for this net)"
+    (at 144.78 96.52 0) (size 30.48 11.43)
     (stroke (width 0) (type default))
     (fill (type none))
     (effects (font (size 1.27 1.27)) (justify left top))
     (uuid a86330ff-260b-47dd-aa97-d0048adacf6a)
   )
+  (text_box "GL4 labels should not raise an error because they have 2 pins"
+    (at 219.71 105.41 0) (size 13.97 15.24)
+    (stroke (width 0) (type default))
+    (fill (type none))
+    (effects (font (size 1.27 1.27)) (justify right top))
+    (uuid af5089b5-a3d7-491f-8ad7-018e7d6a2374)
+  )
+  (text_box "LL3 labels should not raise an error because they have 2 pins"
+    (at 219.71 81.28 0) (size 13.97 15.24)
+    (stroke (width 0) (type default))
+    (fill (type none))
+    (effects (font (size 1.27 1.27)) (justify right top))
+    (uuid b6418cb7-12f9-4487-b0b1-e50751b38650)
+  )
+  (text_box "All three label types should raise the same error when floating"
+    (at 119.38 119.38 0) (size 13.97 15.24)
+    (stroke (width 0) (type default))
+    (fill (type none))
+    (effects (font (size 1.27 1.27)) (justify right top))
+    (uuid d489ff4e-daab-4834-8112-c33606e9ee69)
+  )
   (text_box "Two global labels on the same net should be an error"
-    (at 144.78 95.25 0) (size 30.48 6.35)
+    (at 86.36 96.52 0) (size 30.48 6.35)
     (stroke (width 0) (type default))
     (fill (type none))
     (effects (font (size 1.27 1.27)) (justify left top))
     (uuid e0ed898f-b66d-4e39-bcf1-41f327b1f742)
   )
 
+  (label "LL3" (at 237.49 92.71 0) (fields_autoplaced)
+    (effects (font (size 1.27 1.27)) (justify left bottom))
+    (uuid 3cc27379-d8e5-4bb1-a5ed-ba1a164884ee)
+  )
+  (label "LL2" (at 87.63 124.46 0) (fields_autoplaced)
+    (effects (font (size 1.27 1.27)) (justify left bottom))
+    (uuid 7a1cbb62-a31f-4521-a621-fc5432507df2)
+  )
+  (label "LL2" (at 87.63 118.11 0) (fields_autoplaced)
+    (effects (font (size 1.27 1.27)) (justify left bottom))
+    (uuid 8fe9cea9-eff9-48c3-b409-c27b36a4e9cc)
+  )
   (label "RX1" (at 121.92 71.12 0) (fields_autoplaced)
     (effects (font (size 1.27 1.27)) (justify left bottom))
     (uuid a704bc1b-cffd-42f1-b255-604a994f491f)
   )
+  (label "LL1" (at 135.89 128.27 0) (fields_autoplaced)
+    (effects (font (size 1.27 1.27)) (justify left bottom))
+    (uuid a7a344d0-7a58-4262-83ac-fda394ca4122)
+  )
+  (label "LL3" (at 237.49 85.09 0) (fields_autoplaced)
+    (effects (font (size 1.27 1.27)) (justify left bottom))
+    (uuid a80e6f7e-19a6-4e5a-afff-de51abab7ed9)
+  )
 
   (global_label "GL2" (shape input) (at 124.46 97.79 180) (fields_autoplaced)
     (effects (font (size 1.27 1.27)) (justify right))
@@ -79,8 +206,106 @@
       (effects (font (size 1.27 1.27)) (justify left) hide)
     )
   )
+  (global_label "GL4" (shape input) (at 240.03 119.38 0) (fields_autoplaced)
+    (effects (font (size 1.27 1.27)) (justify left))
+    (uuid 738faa5a-e36d-4864-9c8a-92149eb4280b)
+    (property "Intersheetrefs" "${INTERSHEET_REFS}" (id 0) (at 246.2846 119.38 0)
+      (effects (font (size 1.27 1.27)) (justify left) hide)
+    )
+  )
+  (global_label "GL4" (shape input) (at 240.03 109.22 0) (fields_autoplaced)
+    (effects (font (size 1.27 1.27)) (justify left))
+    (uuid b79dc78c-6af1-460f-9256-36c03a294717)
+    (property "Intersheetrefs" "${INTERSHEET_REFS}" (id 0) (at 246.2846 109.22 0)
+      (effects (font (size 1.27 1.27)) (justify left) hide)
+    )
+  )
+  (global_label "GL3" (shape input) (at 135.89 120.65 0) (fields_autoplaced)
+    (effects (font (size 1.27 1.27)) (justify left))
+    (uuid c7e7b732-ba94-430a-83a4-33f9d3de90b5)
+    (property "Intersheetrefs" "${INTERSHEET_REFS}" (id 0) (at 142.1446 120.65 0)
+      (effects (font (size 1.27 1.27)) (justify left) hide)
+    )
+  )
+
+  (hierarchical_label "HL1" (shape input) (at 135.89 133.35 0) (fields_autoplaced)
+    (effects (font (size 1.27 1.27)) (justify left))
+    (uuid 4abc3818-d024-4893-8971-0b1fee3948ef)
+  )
+
+  (symbol (lib_id "Device:R") (at 91.44 114.3 0) (unit 1)
+    (in_bom yes) (on_board yes) (fields_autoplaced)
+    (uuid 19f41c33-5324-4ee4-ac8a-8893be6f0727)
+    (default_instance (reference "R1") (unit 1) (value "R") (footprint ""))
+    (property "Reference" "R1" (id 0) (at 93.98 113.665 0)
+      (effects (font (size 1.27 1.27)) (justify left))
+    )
+    (property "Value" "R" (id 1) (at 93.98 116.205 0)
+      (effects (font (size 1.27 1.27)) (justify left))
+    )
+    (property "Footprint" "" (id 2) (at 89.662 114.3 90)
+      (effects (font (size 1.27 1.27)) hide)
+    )
+    (property "Datasheet" "~" (id 3) (at 91.44 114.3 0)
+      (effects (font (size 1.27 1.27)) hide)
+    )
+    (pin "1" (uuid 864a3ebe-74a8-448a-b7a8-e239676322f3))
+    (pin "2" (uuid 73d09b4c-2071-4225-a679-dd1d266aa02f))
+  )
+
+  (symbol (lib_id "Device:R") (at 238.76 114.3 0) (unit 1)
+    (in_bom yes) (on_board yes) (fields_autoplaced)
+    (uuid 28d3eb4f-83aa-4d39-b01b-9438757c72eb)
+    (default_instance (reference "R1") (unit 1) (value "R") (footprint ""))
+    (property "Reference" "R1" (id 0) (at 241.3 113.665 0)
+      (effects (font (size 1.27 1.27)) (justify left))
+    )
+    (property "Value" "R" (id 1) (at 241.3 116.205 0)
+      (effects (font (size 1.27 1.27)) (justify left))
+    )
+    (property "Footprint" "" (id 2) (at 236.982 114.3 90)
+      (effects (font (size 1.27 1.27)) hide)
+    )
+    (property "Datasheet" "~" (id 3) (at 238.76 114.3 0)
+      (effects (font (size 1.27 1.27)) hide)
+    )
+    (pin "1" (uuid 77273a01-861b-4502-a01f-04cf8e790056))
+    (pin "2" (uuid 8d04b7d3-2568-4921-ad13-52d09eef47cd))
+  )
+
+  (symbol (lib_id "Device:R") (at 241.3 88.9 0) (unit 1)
+    (in_bom yes) (on_board yes) (fields_autoplaced)
+    (uuid a354c55f-796d-441f-bbf9-83f0447fcf25)
+    (default_instance (reference "R1") (unit 1) (value "R") (footprint ""))
+    (property "Reference" "R1" (id 0) (at 243.84 88.265 0)
+      (effects (font (size 1.27 1.27)) (justify left))
+    )
+    (property "Value" "R" (id 1) (at 243.84 90.805 0)
+      (effects (font (size 1.27 1.27)) (justify left))
+    )
+    (property "Footprint" "" (id 2) (at 239.522 88.9 90)
+      (effects (font (size 1.27 1.27)) hide)
+    )
+    (property "Datasheet" "~" (id 3) (at 241.3 88.9 0)
+      (effects (font (size 1.27 1.27)) hide)
+    )
+    (pin "1" (uuid 4a270a04-22d4-4051-8a5f-22bc82697dc4))
+    (pin "2" (uuid 165af397-576d-4c64-abeb-82e0ba697cda))
+  )
 
   (sheet_instances
     (path "/" (page "1"))
   )
+
+  (symbol_instances
+    (path "/19f41c33-5324-4ee4-ac8a-8893be6f0727"
+      (reference "R1") (unit 1) (value "R") (footprint "")
+    )
+    (path "/a354c55f-796d-441f-bbf9-83f0447fcf25"
+      (reference "R3") (unit 1) (value "R") (footprint "")
+    )
+    (path "/28d3eb4f-83aa-4d39-b01b-9438757c72eb"
+      (reference "R4") (unit 1) (value "R") (footprint "")
+    )
+  )
 )

qa/unittests/eeschema/erc/test_erc_global_labels.cpp

@@ -45,7 +45,7 @@ BOOST_FIXTURE_TEST_CASE( ERCGlobalLabels, ERC_REGRESSION_TEST_FIXTURE )
 
     std::vector<std::pair<wxString, int>> tests =
     {
-        { "issue9367",                   3  }
+        { "issue9367",                   8  }
     };
 
     for( const std::pair<wxString, int>& test : tests )